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

[exporter/datasetexporter]: Add configuration options server_host and retry_shutdown_timeout #24415

Merged

Conversation

martin-majlis-s1
Copy link
Contributor

@martin-majlis-s1 martin-majlis-s1 commented Jul 20, 2023

Description: Introduce server_host configuration to allow specifying server host and retry_shutdown_timeout to specify time for the shutdown function

There is new configuration option server_host that allows specifying how to fill the field ServerHost of the Event structure.

New configuration for server_host looks like this:

    server_host:
      # If these attributes are not specified or empty,
      # use the value from the env variable SERVER_HOST
      use_host: ${env:SERVER_HOST}
      # If it's not set, use the hostname value
      use_hostname: true

This configuration is applied for logs and traces.

Since it's touching the same code, I have already merged changes from this PR - #23881 - into this branch, hoping that it will be merged soon. It's still not merged, so it's blocked by this.

New configuration for retry_shutdown_timeout looks like this:

    buffer:
      # Send buffer to the API at least every 5s
      max_lifetime: 5s
      # Group data based on these attributes
      group_by:
        - attributes.container_id
        - container_id
      # try to send data to the DataSet for at most 30s during shutdown
      retry_shutdown_timeout: 30s

This configuration specifies how much time the shutdown function has to send all the data that it has already accepted.

Link to tracking Issue: #24252

Testing:

I have added several unit tests to make sure that it works as expected.

Documentation: There is new section in the README describing the newly added configuration options.

zdaratom-s1 and others added 30 commits June 7, 2023 14:49
observed timetamp in case LogRecord doesn't contain timestamp.

Even though ObservedTimestamp should always be present, we fall back to
current time in case it's not.

In addition to that, remove duplicate and redundant "timestamp"
attribute from the AddEvents event.
DSET-3998 feat: export Logs resource info based on export_resource_info_on_event configuration
…try-collector-contrib into event_no_timestamp_fix_use_occurence_time
…e_time

[DSET-4071] [DSET-4007] Fix timestamp handling for LogRecord objects without a timestamp
severity to DataSet severity.

Also remove "severity.text" and "severity.number" field from the event
since it's redundant - we already have event severity (sev) field value.
information which is not already available (events which are sampled
will already be ingested and visible in the ui so flags.is_sampled is
redundant).
functions. Also move functionality for making a test record into a
utility function.
[DSET-4037] [DSET-4014] [DSET-4034] [DSET-4034] [DSET-4098] Dataset log exporter improvements
@github-actions github-actions bot requested a review from tomaz-s1 July 25, 2023 15:07
@martin-majlis-s1
Copy link
Contributor Author

This is the failing job - https://github.com/open-telemetry/opentelemetry-collector-contrib/actions/runs/5679983329/job/15393224752?pr=24415

When I run the command locally - make -j2 golint GROUP=other it succeeds.

Copy link
Contributor

@atoulme atoulme left a comment

Choose a reason for hiding this comment

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

LGTM but please follow up on Curtis's feedback, as right now you have 2 changelog entries. You should have one.

@martin-majlis-s1 martin-majlis-s1 changed the title [exporter/datasetexporter]: Extract serverHost [exporter/datasetexporter]: Add configuration options server_host and retry_shutdown_timeout Jul 31, 2023
@martin-majlis-s1
Copy link
Contributor Author

LGTM but please follow up on Curtis's feedback, as right now you have 2 changelog entries. You should have one.

Since this PR is in progress for 3 weeks I have added here one more change. Therefore there are two entries. I know, that it's not ideal, but if the second change is trivial - pass another configuration option to the library, it's fine to bundle it here as well.

Copy link
Contributor

@zdaratom-s1 zdaratom-s1 left a comment

Choose a reason for hiding this comment

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

LGTM

@mx-psi mx-psi merged commit 8dc9eaf into open-telemetry:main Aug 2, 2023
88 of 89 checks passed
@github-actions github-actions bot added this to the next release milestone Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cmd/configschema configschema command cmd/otelcontribcol otelcontribcol command exporter/dataset
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants