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

[connector/exceptions] Always add span name as a dimension in the output metrics and log records #32211

Merged

Conversation

niftyhoot
Copy link
Contributor

Description:
This change adds span.name as one of the default fields attached to the output logs and metrics.
This can help isolate the root cause of problems to specific endpoints within a service.

Link to tracking Issue:
#32162

Testing:

Unit Test Updated and Passed.
Verified by label by sending the metrics to the prometheus exporter.
Verified that logs output has span.name by sending the output to Splunk HEC exporter

Documentation:
Updated README.md and Changelog file

@marctc
Copy link
Contributor

marctc commented Apr 8, 2024

Overall looks good, thanks!! I have a question before approving: is it any case that span names are dynamically generated? I'm concern if that happens in sort of scenarios in can led to cardinality explosion.

@niftyhoot
Copy link
Contributor Author

Overall looks good, thanks!! I have a question before approving: is it any case that span names are dynamically generated? I'm concern if that happens in sort of scenarios in can led to cardinality explosion.

Very good question. It is quite possible and I was planning to propose the exclude dimension option soon after this PR.
Current default dimensions of exception.message is actually a far greater risk than span.name
Very common to see volatile details and multi-line contents in exception.message label and systems like Prometheus dont like labels with long string values.

However, a mechanism wherein dimensions for Logs and dimensions for Metrics outputs are independently listed and the ability to ignore (on a per services basis) where one or more such dimension can result in cardinality explosions is certainly needed. Ideally, it should be a mix of dynamic (subject to some sort of total cardinality limit and based on past heuristics) and static wherein a rogue service is explicitly listed to be use a non-default dimension set. Happy to create a GH issue and publish a PR if you'd be okay with that.

@niftyhoot
Copy link
Contributor Author

@marctc @jpkrohling @bogdandrutu - using the new-comer card and thus taking the liberty to ask a dumb question - what should be the next step here? would someone from code-owners group run any additional tests and merge this automatically ?

Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Apr 26, 2024
@github-actions github-actions bot removed the Stale label May 1, 2024
@niftyhoot
Copy link
Contributor Author

@marctc @jpkrohling @bogdandrutu - can one you please help with merging of this PR ?

@jpkrohling jpkrohling merged commit 63d2489 into open-telemetry:main May 7, 2024
162 checks passed
@github-actions github-actions bot added this to the next release milestone May 7, 2024
@niftyhoot niftyhoot deleted the always-add-span-name-dimension branch May 8, 2024 03:55
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.

4 participants