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

OTEL transaction naming | Identity transaction per METHOD protocol://host:port/path #11762

Closed
ludovic-pourrat opened this issue Oct 14, 2023 Discussed in #11761 · 12 comments
Closed
Assignees
Labels
pending author feedback Waiting for the issue author to get back to a maintainer with findings, more details, etc... stale

Comments

@ludovic-pourrat
Copy link

ludovic-pourrat commented Oct 14, 2023

Discussed in #11761

Originally posted by ludovic-pourrat October 14, 2023
As of today when enabling the OTEL plugin into Kong, all the transactions are named statically "kong" where usually the OTEL agents pick up a transaction identification that reflects better the transactions. In the case of Kong the transition should be something related to the route exposed.

This feature request aim to provide an addition property to named the OTEL transaction differently based on the route definition to follow a common pattern in the OTEL, e.g. METHOD protocol://host:port/path.

I will provide a pull request to illustrate this.

Feedback, and advices are welcome !

ludovic-pourrat added a commit to ludovic-pourrat/kong that referenced this issue Oct 14, 2023
@ludovic-pourrat
Copy link
Author

ludovic-pourrat commented Oct 14, 2023

See https://github.com/ludovic-pourrat/kong/tree/feat/otel-transaction-naming as draft proposal.

We successfully tested this change today and we are able to track the transactions as expected.

@nowNick
Copy link
Contributor

nowNick commented Oct 16, 2023

@samugi Could you take a look at this proposal?

ludovic-pourrat added a commit to ludovic-pourrat/kong that referenced this issue Oct 16, 2023
@ludovic-pourrat ludovic-pourrat changed the title OTEL transaction naming | Identity transaction per protocol://host:port/path OTEL transaction naming | Identity transaction per METHOD protocol://host:port/path Oct 16, 2023
@samugi
Copy link
Member

samugi commented Oct 24, 2023

Similar issue: #11511

Span names, aka "operation names" were updated in pull/10577 to satisfy the need of having a lower cardinality on certain platforms (Datadog in particular), where having the "path" define different operations resulted in what some users considered to be too many different operations that made searching and grouping of spans challenging.

Since different users have different requirements, as mentioned in #11511, one way to improve this could be increasing flexibility by making the span names customizable, rather than assigning a fixed / hardcoded value. We can consider that as a future enhancement, but it is not planned yet. For this reason at this time we cannot take the approach requested in this issue as a valid way forward.

Please let me know if you have any further comments or doubts and I will do my best to address them.

@samugi samugi added the pending author feedback Waiting for the issue author to get back to a maintainer with findings, more details, etc... label Oct 24, 2023
@ludovic-pourrat
Copy link
Author

Hi,

Thanks for your feedback, by the way if you take the point of view of the open telemetry, there are not that much of a user customisation requirement when dealing with semantics on http and messaging traces (that applies to the nature of a multi-protocol API gateway). Further-more, if you look on the top vendors in this space (OTEL) they typically do not offer a way to customise the spans names too.

So, I would rather advocate for an opinionated choice to name the spans according to the OTEL guidelines, see references below.

https://opentelemetry.io/docs/specs/semconv/
https://opentelemetry.io/docs/specs/semconv/http/http-spans

At this stage the current implementation if not applicable as it mixes all the transactions into the "kong" name, but we can afford to wait with our patch approach.

How will be the way to track this topic later-on if this proposal got rejected ?

Thanks again.

@samugi
Copy link
Member

samugi commented Oct 24, 2023

you raise a more than valid point @ludovic-pourrat. I will look into this a bit further and come back with some feedback / proposal. Thanks for bringing this up.

@samugi samugi removed the pending author feedback Waiting for the issue author to get back to a maintainer with findings, more details, etc... label Oct 24, 2023
@samugi
Copy link
Member

samugi commented Oct 25, 2023

@ludovic-pourrat
Going by the guidelines:

HTTP server span names SHOULD be {method} {http.route} if there is a (low-cardinality) http.route available [...]. If there is no (low-cardinality) http.route available, HTTP server span names SHOULD be {method}.

I can think of three main formats that could be desirable for the kong root span's name in particular:

  1. kong (current) - identifies the root span of the trace generated by Kong
  2. {method} {http.route} - based on the guidelines, for low-cardinality http.route
  3. {method} - based on the guidelines, for high-cardinality http.route

A safe default would be just {method}, to account for all those configurations where the {http.route} cardinality is too high.
There is a common requirement of having kong as part of the span name, to make Kong traces recognizable in distributed tracing scenarios, so perhaps a combination of 1 and 3 could be a good (non-configurable) default: kong {method}. I have the feeling that wouldn't satisfy the requirement described in this issue in particular, though. At the same time including http.route by default cannot be done to avoid the high cardinality issues mentioned above that we already know to be problematic for some.

Maybe a good compromise would be leaving kong as the default (avoid introducing a breaking change), and add a configuration option that allows appending {method} and {http.route} individually to adapt to different route cardinality scenarios and needs, what do you think?

Quick note: I looked into the proposed solution here, it seems that it goes against the specification since it includes the full URI (including the path) in the span name:

Instrumentation MUST NOT default to using URI path as span name

@samugi samugi added the pending author feedback Waiting for the issue author to get back to a maintainer with findings, more details, etc... label Oct 25, 2023
@ludovic-pourrat
Copy link
Author

Hi,

First of all, I would not advocate for having kong in the naming of the transactions just because it add a unnecessary context into the transactions where in OTEL the services a.k.a Kong can be queried and selected by the resource attributes. In that idea you would not expect Quarkus or Springboot to tag their transactions too.
Secondly, whatever the cardinality is in the scope of Kong it just reflect the traffic and the transactions managed by the gateway and it should always be sufficiently differenticiated with {method} and {http.route}, so I would go for option 2, as we consider the option 3 to be useless at the end.

You are right our proposal is also adding the {path}, by the way as the gateway is not capable to understand the service router (getting knowledge about the path params) it will lead to too many cardinalities. So it clearly highlight why the {path} should not be part of the named transaction in the case of a gateway. We will review our proposal to take this as an outcome of our analysis to remove the {path} from it.

Thanks for your feedback.

@samugi samugi removed the pending author feedback Waiting for the issue author to get back to a maintainer with findings, more details, etc... label Oct 30, 2023
@samugi
Copy link
Member

samugi commented Oct 30, 2023

@ludovic-pourrat I agree with most of your considerations, there are two details I want to highlight:

  1. I think it's important to honor the "routes are too many to define span names" scenario, as the Opentelemetry guidelines suggest: If there is no (low-cardinality) http.route available, HTTP server span names SHOULD be {method}. so while I agree the {method} {http.route} option provides the most meaningful operation name, I would argue we should still provide a way to add/remove the route from the name
  2. There've been requests to set specific names to spans, that led to the current implementation, which cannot be broken

Because of that, I think an acceptable solution would be to provide a configuration option with a string pattern that can optionally include the method and route parameters, to provide maximal flexibility. If you wish to take care of such implementation that would be great, otherwise I can create an internal item to track this and then link this issue once there will be a PR.

@samugi samugi added the pending author feedback Waiting for the issue author to get back to a maintainer with findings, more details, etc... label Oct 30, 2023
@ludovic-pourrat
Copy link
Author

@samugi fine, I will make a PR following your last proposal.

Thanks !

@samugi
Copy link
Member

samugi commented Oct 31, 2023

that's awesome @ludovic-pourrat thank you! Feel free to ping me when it's ready, so we can review it. Thanks again.

Copy link
Contributor

This issue is marked as stale because it has been open for 14 days with no activity.

@github-actions github-actions bot added the stale label Nov 15, 2023
Copy link
Contributor

Dear contributor,

We are automatically closing this issue because it has not seen any activity for three weeks.
We're sorry that your issue could not be resolved. If any new information comes up that could
help resolving it, please feel free to reopen it.

Your contribution is greatly appreciated!

Please have a look
our pledge to the community
for more information.

Sincerely,
Your Kong Gateway team

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Nov 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pending author feedback Waiting for the issue author to get back to a maintainer with findings, more details, etc... stale
Projects
None yet
Development

No branches or pull requests

3 participants