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.

Sign up for GitHub

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

Jump to bottom

feat(instrumentation): fix high cardinality span name #10577

Merged
merged 6 commits into from
Apr 17, 2023
Merged

feat(instrumentation): fix high cardinality span name #10577

merged 6 commits into from
Apr 17, 2023

Conversation

StarlightIbuki
Copy link
Contributor

@StarlightIbuki StarlightIbuki commented Mar 29, 2023 •
edited by samugi
Loading

Summary

High cardinality span names (operation name for Datadog) causes the spans unable to group, and not able to help analysis in the dashboard.

We reorgnize the span names, to handle the cardinality with "resource" field for Datadog. This PR is to modified shared code for both CE and EE involved in this fix

Checklist

  • The Pull Request has tests
  • There's an entry in the CHANGELOG
  • [not needed] There is a user-facing docs PR against https://github.com/Kong/docs.konghq.com - PUT DOCS PR HERE
  • This is a breaking change. We need to see what this looks like on OTEL dashboard and evaluate the change

Full changelog

  • Add comments to clarify the definition of spans
  • Rename spans

Issue reference

Fix FTI-4882
KAG-1126

@StarlightIbuki StarlightIbuki self-assigned this Mar 29, 2023
@StarlightIbuki StarlightIbuki force-pushed the refactor/tracing-span-design branch 2 times, most recently from ea17c5c to 797d852 Compare March 29, 2023 07:30
@bungle bungle changed the title feat(instrumentation): fix high cardnality span name feat(instrumentation): fix high cardinality span name Mar 29, 2023
@hbagdi
Copy link
Member

hbagdi commented Mar 29, 2023

What is the next step on this PR?

@StarlightIbuki
Copy link
Contributor Author

What is the next step on this PR?

First, we should evaluate the change. The noticeable change is that the root span name of OTEL changes from GET /?key=value to proxy.
Then, I will finish the resource name part for Datadog (on EE).

@StarlightIbuki
Copy link
Contributor Author

@hbagdi And if possible, we should do this quickly. The customer wants this change before next week, which leaves us 2 days for evaluating and reviewing/merging both PRs, and verification of the fix.

@fffonion
Copy link
Contributor

fffonion commented Mar 30, 2023 •
edited
Loading

@StarlightIbuki is there a existing case we can learn about naming pattern for spans? (For example, what does nginx's opentelemtry module do?) Also could you share the difference (either in a dashboard or in your words) before and after the change? I'm asking this because most of us doesn't have sufficient knowledge on this area, and I would like us to learn from examples and not just making decisions we thought were reasonable.

@StarlightIbuki
Copy link
Contributor Author

@StarlightIbuki is there a existing case we can learn about naming pattern for spans?

@fffonion The default value for span names and resource names: https://github.com/DataDog/nginx-datadog/#:~:text=Create%20one%20span%20per%20request

@samugi samugi force-pushed the refactor/tracing-span-design branch from b7e5332 to 93c4f27 Compare April 4, 2023 13:39
@hbagdi
Copy link
Member

hbagdi commented Apr 5, 2023

There's an entry in the CHANGELOG

That item is checked but there isn't any changelog entry.

hbagdi
hbagdi reviewed Apr 5, 2023
View reviewed changes
kong/tracing/instrumentation.lua Outdated Show resolved Hide resolved
kong/tracing/instrumentation.lua Outdated Show resolved Hide resolved
kong/tracing/instrumentation.lua Show resolved Hide resolved
@samugi samugi force-pushed the refactor/tracing-span-design branch from 93c4f27 to f7a54a0 Compare April 6, 2023 07:51
samugi
samugi requested changes Apr 15, 2023
View reviewed changes
spec/02-integration/14-tracing/01-instrumentations_spec.lua Outdated Show resolved Hide resolved
kong/plugins/opentelemetry/handler.lua Outdated Show resolved Hide resolved
kong/tracing/instrumentation.lua Outdated Show resolved Hide resolved
StarlightIbuki and others added 4 commits April 17, 2023 10:25
@StarlightIbuki @samugi
chore(tracing): clarify def of span struct
61af6f9
@StarlightIbuki @samugi
feat(instrumentation): rename spans to lower card
12bd224 
rename spans to ensure lower cardinality
initially spans included dymanic information in their names (path) that
produced a large range of information that made filtering difficult
@StarlightIbuki @samugi
apply suggestions on naming
e521bc3
@samugi
chore(instrumentation): reword span names and cleanup
450182a 
remove .internal from the span names prefixes where not needed
clean up comments that are not needed
@samugi samugi force-pushed the refactor/tracing-span-design branch from af38391 to 5e58a63 Compare April 17, 2023 08:35
samugi
samugi approved these changes Apr 17, 2023
View reviewed changes
Copy link
Member

@samugi samugi left a comment

Choose a reason for hiding this comment

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

we've kept this open for a while to discuss the chosen names, which have been updated accordingly, merging this now.

@samugi samugi merged commit 14ecc08 into master Apr 17, 2023
@samugi samugi deleted the refactor/tracing-span-design branch April 17, 2023 09:30
@samugi samugi mentioned this pull request Oct 24, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hbagdi hbagdi hbagdi left review comments

@samugi samugi samugi approved these changes

Assignees

@StarlightIbuki StarlightIbuki

Labels
core/pdk core/tracing size/M
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@StarlightIbuki @hbagdi @fffonion @samugi