Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

out_es: Add target_index variable using record accessor syntax. #7716

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Wiston999
Copy link

@Wiston999 Wiston999 commented Jul 18, 2023

Add target_index variable for rendering ES index name using record accessor syntax. This allows to define ES index name based on record accessor, enabling to use record field values as destination ES index.
It differs from logstash_prefix_key as no time component is added to index name, so ES data streams can be used as target index easily.

Tests output and other information at https://gist.github.com/Wiston999/c7d3ff5389f64fdf1113146090ab1aa1.

Addresses #2514


Enter [N/A] in the box, if an item is not applicable to your change.

Testing
Before we can approve your change; please submit the following in a comment:

  • Attached Valgrind output that shows no leaks or memory corruption was found (here)

If this is a change to packaging of containers or native binaries then please confirm it works for all targets.

  • [N/A] Run local packaging test showing all targets (including any new ones) build.
  • [N/A] Set ok-package-test label to test for all targets (requires maintainer to do).

Documentation

  • Documentation required for this feature

fluent/fluent-bit-docs#1163

Backporting

  • [N/A] Backport to latest stable release.

Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.

@patrick-stephens
Copy link
Contributor

I think for Opensearch, Index was just updated to support RA syntax - would it be better to do that for ES as well rather than a new key?

@Wiston999 Wiston999 temporarily deployed to pr July 18, 2023 13:42 — with GitHub Actions Inactive
@Wiston999 Wiston999 temporarily deployed to pr July 18, 2023 13:42 — with GitHub Actions Inactive
@Wiston999
Copy link
Author

I think for Opensearch, Index was just updated to support RA syntax - would it be better to do that for ES as well rather than a new key?

I considered that option too. I'm open to change to RA on Index and adding a new Index_default or whatever variable name for a fallback index name in case RA doesn't render any value.

Just confirm me if you prefer that option and I'll make the changes.

@Wiston999
Copy link
Author

I've updated the PR to use the same approach as out_opensearch plugin, it is, detect if Index variable has $ and treat it like a RA expression.

Updated gist .

@Wiston999 Wiston999 temporarily deployed to pr July 20, 2023 09:09 — with GitHub Actions Inactive
@Wiston999 Wiston999 temporarily deployed to pr July 20, 2023 09:09 — with GitHub Actions Inactive
@Wiston999 Wiston999 temporarily deployed to pr July 20, 2023 09:09 — with GitHub Actions Inactive
@Wiston999 Wiston999 temporarily deployed to pr July 20, 2023 09:40 — with GitHub Actions Inactive
@DandyDeveloper
Copy link

Anybody looking at this? I'm also impacted and seems as though opensearch as an alternative broke with the API changes in recent release, so I can't use that as a bandaid.

👍

@braydonk
Copy link
Contributor

What happened with the commits on this PR? Whatever happened last week, where it added a bunch of commits from master, should probably be resolved. Only the 3 out_es commits that are actually part of this PR should be here.

@Wiston999 Wiston999 force-pushed the out-es-target-index-key branch from fd14fe2 to 8a6a409 Compare August 16, 2023 08:29
@Wiston999
Copy link
Author

What happened with the commits on this PR? Whatever happened last week, where it added a bunch of commits from master, should probably be resolved. Only the 3 out_es commits that are actually part of this PR should be here.

I've just fixed the mess with commits from master. I'm sorry for the noise introduced in the PR and for the inconveniences to other committers.

@Wiston999 Wiston999 temporarily deployed to pr August 16, 2023 15:07 — with GitHub Actions Inactive
@Wiston999 Wiston999 temporarily deployed to pr August 16, 2023 15:07 — with GitHub Actions Inactive
@Wiston999 Wiston999 temporarily deployed to pr August 22, 2023 10:19 — with GitHub Actions Inactive
@Wiston999 Wiston999 temporarily deployed to pr August 22, 2023 10:19 — with GitHub Actions Inactive
@Wiston999 Wiston999 temporarily deployed to pr August 22, 2023 10:19 — with GitHub Actions Inactive
@Wiston999 Wiston999 temporarily deployed to pr August 22, 2023 10:52 — with GitHub Actions Inactive
@DandyDeveloper
Copy link

DandyDeveloper commented Aug 29, 2023

@Wiston999

[2023/08/29 13:22:37] [ info] [fluent bit] version=2.1.9, commit=6530852f3e, pid=1
[2023/08/29 13:22:37] [ info] [storage] ver=1.4.0, type=memory, sync=normal, checksum=off, max_chunks_up=128
[2023/08/29 13:22:37] [ info] [cmetrics] version=0.6.3
[2023/08/29 13:22:37] [ info] [ctraces ] version=0.3.1
[2023/08/29 13:22:37] [ info] [input:kafka:kafka.0] initializing
[2023/08/29 13:22:37] [ info] [input:kafka:kafka.0] storage_strategy='memory' (memory only)
[2023/08/29 13:22:37] [engine] caught signal (SIGSEGV)
#0  0x7fd499c0cab8      in  ???() at 4/multiarch/strlen-evex.S:77
#1  0x556670fbd58e      in  flb_strdup() at include/fluent-bit/flb_str.h:46
#2  0x556670fc127f      in  flb_upstream_create() at src/flb_upstream.c:286
#3  0x5566712f5712      in  flb_es_conf_create() at plugins/out_es/es_conf.c:230
#4  0x5566712eaef8      in  cb_es_init() at plugins/out_es/es.c:658
#5  0x556670f7ce31      in  flb_output_init_all() at src/flb_output.c:1301
#6  0x556670fa122e      in  flb_engine_start() at src/flb_engine.c:787
#7  0x556670f4526a      in  flb_lib_worker() at src/flb_lib.c:638
#8  0x7fd49a2e2ea6      in  start_thread() at reate.c:477
#9  0x7fd499b96a2e      in  ???() at sysv/linux/x86_64/clone.S:95
#10 0xffffffffffffffff  in  ???() at ???:0

Getting this segfault when using this kind of output using your branch + master merged

      [OUTPUT]
          Name                es
          Match               *
          Host                ${FLUENT_ELASTICSEARCH_HOST}
          Port                ${FLUENT_ELASTICSEARCH_PORT}
          HTTP_User           ${FLUENT_ELASTICSEARCH_USERNAME}
          HTTP_Passwd         ${FLUENT_ELASTICSEARCH_PASSWORD}
          Id_Key              id
          Index               $topic

Any ideas if something you've changed broke this?

@Wiston999
Copy link
Author

Hi @DandyDeveloper ,
the line of code throwing the seg fault seems https://github.com/fluent/fluent-bit/blob/master/src/flb_upstream.c#L286 , which seems to be related to some proxy configuration.

@DandyDeveloper
Copy link

Any ideas on when this might get merged in?

@DandyDeveloper
Copy link

@edsiper @leonardo-albertovich Can we get some eyes on this please?

@ajax-bychenok-y
Copy link

ajax-bychenok-y commented Oct 9, 2023

@DandyDeveloper I came up with next idea, maybe it will be useful for you. It's clusteroutput for K8s fluentbit-operator resource but I think it's quite easily can be transformed to just daemon config (check spec section):

apiVersion: fluentbit.fluent.io/v1alpha2
kind: ClusterOutput
metadata:
  annotations:
    meta.helm.sh/release-name: fluent-operator
    meta.helm.sh/release-namespace: fluent
  labels:
    app.kubernetes.io/managed-by: Terraform
    fluentbit.fluent.io/component: logging
    fluentbit.fluent.io/enabled: "true"
  name: es
spec:
  es:
    bufferSize: 50MB
    generateID: true
    host: ${output_es_host}
    # That's a hack for not creating datastreams with date suffix
    logstashDateFormat: eks
    logstashFormat: true
    # In this index go all records w/o proper label 
    logstashPrefix: company-unknown
    logstashPrefixKey: kubernetes['labels']['app.kubernetes.io/name']
    port: 9200
    replaceDots: false
    suppressTypeName: "On"
    timeKey: '@timestamp'
  matchRegex: (?:kube|service)\.(.*)

So fluentbit sends everything in kubernetes['labels']['app.kubernetes.io/name'] datastream name (index template with datastream creating option needs to be previously turned on) and adds eks suffix in the end. Yes, it's quite hacky.

@DandyDeveloper
Copy link

@ajax-bychenok-y Thanks for the suggestion! This could work in theory actually, I'll have a crack. But, I'm a little shocked this PR has been pending for so long... It's a pretty simple addition.

Victor Cabezas added 3 commits December 4, 2023 12:10
Add target_index variable for rendering ES index name using record
accessor syntax without extra bytes (i.e.: no time component on index
name).

Signed-off-by: Victor Cabezas <[email protected]>
@Wiston999
Copy link
Author

Hi @edsiper or any other maintainer. Are these changes being considered to be merged?
If so, is there any pending action from my side to be taken that is preventing the PR to be merged?

We would like to start upgrading our ES clusters to v8 and as @DandyDeveloper pointed out these changes are needed to have index RA behavior on ES v8 clusters.

Copy link
Contributor

github-actions bot commented Mar 8, 2024

This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days.

@github-actions github-actions bot added the Stale label Mar 8, 2024
@DandyDeveloper
Copy link

Anyone alive

@2nfree
Copy link

2nfree commented Jun 28, 2024

Is anyone else reviewing or keeping track of this commit?

@cw-Guo
Copy link
Contributor

cw-Guo commented Jul 16, 2024

Hi @cosmo0920 @edsiper , is there anything else needed to do in this pr before it can be merged?

(char *) tag, tag_len,
map, NULL);
if (v) {
len = flb_sds_len(v);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

either dynamically allocating the index so it is not truncated or returning a warning or error might be better idea. The limit should also be checked against the actual size of index_value, (sizeof()-1), instead of a hardcoded value.

Copy link
Contributor

This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days.

@github-actions github-actions bot added the Stale label Nov 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants