[chore] [exporter/datasetexporter] Simplify trace exporter, get rid of the client side aggregation code #23881
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
aggregation which is not needed anymore (populating services, span_count and error_count attributes on the client side). This aggregation now happens on the server side and implementing it on the client side was always meant as a short term workaround early in the development cycle since this approach had many edge cases and only would work correctly / as expected in some limited situations.
as a temporary workaround until a proper approach is implemented on the server side.
tomaz-s1
commented
Jul 3, 2023
| # you should instead start your pull request title with [chore] or use the "Skip Changelog" label. | ||
|
|
||
| # One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' | ||
| change_type: breaking |
There was a problem hiding this comment.
I labeled it as breaking even though plugin is still in alpha stage and some breaking changes can be expected in alpha stage. Let me know if this should be changed.
tomaz-s1
changed the title
martin-majlis-s1
approved these changes
Jul 4, 2023
|
@atoulme Please let me know if there is anything I can do / help with, with regards to the review process. Thanks. |
atoulme
approved these changes
Jul 11, 2023
mx-psi
pushed a commit
that referenced
this pull request
Aug 2, 2023
… retry_shutdown_timeout (#24415) **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. --------- Co-authored-by: tomas.zdara <[email protected]> Co-authored-by: Tomaz Muraus <[email protected]> Co-authored-by: Tomaz Muraus <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request updates trace exporter code and gets rid of client side aggregation / populating of
services,span_countanderror_countattributes.This client side aggregation is not needed anymore since it now happens on the server side.
It's worth noting that this client side aggregation was meant as a quick short term workaround / hack early in the development cycle (special thanks to @martin-majlis-s1 for implementing this workaround so quickly on the client side).
It was never meant as a long term solution since it had multiple edge cases and problems associated with it - it would only really work correctly / as expected in case all the spans which belong to the same trace are part of the same batch received by the plugin. And of course that won't be true in many scenarios - different spans are routed to different collectors, long running spans which are not part of the same batch, no guarantees how batches are assembled, etc.
As part of this change,
traces.aggregateandtraces.max_waitconfig options have also been made obsolete / redundant.Those config options have also been removed - since the plugin is still an alpha (some breaking changes are expected) we still have the luxury of being able to do that.
Removing this code also makes the plugin code much simpler now - and who doesn't like removing code and simplifying things (less thing to maintain and worry about edge cases, etc.) :)