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

feat(tracing): route/service scoping #10096

Merged
merged 4 commits into from
Jan 17, 2023
Merged

Conversation

StarlightIbuki
Copy link
Contributor

implement KAG-244

kong/tracing/tracer.lua Outdated Show resolved Hide resolved
@StarlightIbuki StarlightIbuki force-pushed the feat/router_scoped_tracing branch 2 times, most recently from 8ad190e to 91f4f19 Compare January 11, 2023 10:30
@StarlightIbuki StarlightIbuki marked this pull request as ready for review January 16, 2023 03:09
@fffonion
Copy link
Contributor

I feel like we are mixing the source of truth of "whether to trace or not" in different places. Previously when
otel plugin can only be global:

  1. we have a opentelemetry_tracing config that tells Kong whether to collect specific instrumentations
  2. we have otel plugin that controls whether and where a collected spans needs to be flushed to

In this PR we have an additional source of truth on "whether to trace or not" depending on if any of a tracing
plugin (otel in this case) is enabled on current path, and that conflicts with fact that it's previously only
controlled by opentelemetry_tracing. So i would suggest we refactor the implementation a bit so that:

  1. we still rely on opentelemetry_tracing config that tells Kong whether to collect specific instrumentations
  2. rely on plugins_iterator to pick a plugin on current request, if such otel plugin exists, then its log phase
    phase handler is executed, and spans are flushed.

so that we don't need complex logic over iterration over plugins and cache invalidations.

@StarlightIbuki
Copy link
Contributor Author

I feel like we are mixing the source of truth of "whether to trace or not" in different places. Previously when otel plugin can only be global:

  1. we have a opentelemetry_tracing config that tells Kong whether to collect specific instrumentations
  2. we have otel plugin that controls whether and where a collected spans needs to be flushed to

In this PR we have an additional source of truth on "whether to trace or not" depending on if any of a tracing plugin (otel in this case) is enabled on current path, and that conflicts with fact that it's previously only controlled by opentelemetry_tracing. So i would suggest we refactor the implementation a bit so that:

  1. we still rely on opentelemetry_tracing config that tells Kong whether to collect specific instrumentations
  2. rely on plugins_iterator to pick a plugin on current request, if such otel plugin exists, then its log phase
    phase handler is executed, and spans are flushed.

so that we don't need complex logic over iterration over plugins and cache invalidations.

🤦‍You are right. The code could be much more straightforward. I am rewriting the code.

@pull-request-size pull-request-size bot added size/L and removed size/M labels Jan 16, 2023
@pull-request-size pull-request-size bot added size/M and removed size/L labels Jan 16, 2023
@fffonion fffonion merged commit 64fabec into master Jan 17, 2023
@fffonion fffonion deleted the feat/router_scoped_tracing branch January 17, 2023 08:27
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.

3 participants