open-telemetry / opentelemetry-js-contrib

OpenTelemetry instrumentation for JavaScript modules
https://opentelemetry.io
Apache License 2.0
701 stars 516 forks source link

Span hierarchy if HTTP Instrumentation is not installed #466

Closed rauno56 closed 2 years ago

rauno56 commented 3 years ago

We should agree on how we expect a web framework instrumentation creates the span hierarchy.

Most Node.js frameworks have a list of function handlers per request. Each of those pieces of work usually gets a span associated with it, which makes sense - as a user, I'd definitely want to know how often each of my handlers logically representing a part of a work required to respond to a request are hit, the time it takes to execute each one of them, etc. As a user, I'd also definitely expect that each instrumentation does it in a consistent way. There are currently no guidelines on this AFAIK, and different instrumentations behave differently.

If HTTP instrumentation is installed. There is no question. The most obvious way is to nest all handlers under it:

HTTP:      [ HTTP Span for the request ]
Framework:  [^ H1 ][^    H2    ][^ H3 ]

H* are handler spans for one single request. All handler spans are children of the one from the HTTP layer.

What's the expected behavior if HTTP instrumentation is not installed?

Options:

  1. Do nothing.
{ ...Crickets... }

HTTP Instrumentation is required. The argument might be for avoiding useless edge-cases and only deal with the most common one.

Opinion: I don't think it's intuitive. Any instrumentation should come with batteries included. If we consider the missing HTTP Inst as a "misconfiguration"(which would be justifiable), it should at least error at boot time or make it really obvious by some other means so that the operator would be at least informed.

Express and Hapi Instrumentations currently behave this way being a no-op, if there's no parent span.

  1. Create parentless spans.
Framework:  [ H1 ][     H2     ][  H3 ]

HTTP Instrumentation is not required, but the utility of a framework inst. is significantly degraded. Trace propagation would not work for example. The operator gets individual spans for each of the handlers, but those would not be connected to any request nor each other.

Opinion: It's better than nr.1 because the framework instrumentation at least does as much as it can without exiting it "responsibility area". The generated spans also implicitly communicate the absence of HTTP Inst. However, I don't see anyone desperately wanting to use a framework inst in such a way.

No Instrumentation I know currently behaves this way, but the ones that follow nr.1 could easily be converted.

  1. Make a span be a child of the previous handler's span
Framework:  [ H1 ]
                  [^    H2     ]
                                [^ H3 ]

HTTP Instrumentation is not required, the utility of the framework inst. is significantly degraded and the structure of the spans is different. However, it's at least possible to derive the "shape" of the request, since, unlike others, the first handler of every request is parentless.

Opinion: There are pros to this option, but the fact that it's structured differently is a deal-breaker. Spans with and without HTTP Inst installed should at least resemble each other.

Restify's Instrumentation does this, if there's no parent span.


In conclusion, I'd argue for nr. 2, but are curious about everyone's thoughts on that. I don't think it's specific to Node.js, and wonder whether anyone has come across to a similar discussion in the context of other languages?

Flarna commented 3 years ago

There is one more case which should be included in such a discussion:

HTTP doesn't create a span because the ignoreIncomingPaths configuration tells a specific request (e.g. health requests,...) should not create a span. Such configs are usually done to avoid overhead and/or focus on the important stuff. It would be not that nice if followup frameworks like express create 1 or N traces even a user configured HTTP to skip trace creation.

rauno56 commented 3 years ago

That's a good point! I guess that's the main argument for the current behaviour of the instrumentations of Express and Hapi?

Taking that into account there has to be a well-known way to track that into the depth of the request and I can also see a similar situation arising in other layers of request handling as well.

How does ignoreIncomingPaths interact with the behaviour and requirements of sampling those spans out altogether? I assume sampling already handles cases of nested spans, etc. The overhead of creating the spans still stays(not ideal), but at least those would not exit the processes boundaries.

Flarna commented 3 years ago

If I understand the code correct ignoreIncomingPaths should suppress all child spans.

For sampling it depends. If you use a parent based sampler the decision of the parent is used also for the child. For other samples (e.g. pure random) the decisions are independent. So non recorded spans are just "missing" and child spans may point to a non existing parent.

vreynolds commented 3 years ago

There was a good discussion about this in the SIG, going to brain dump some notes here for posterity. (please correct if I got anything wrong)

Historically, instrumentation authors made the decision about this behavior, since it’s not defined in the spec. For example, would the individual spans be useful (make sense) to end user when there is no parent span?

Should there be explicit dependencies between instrumentations that require one another? http/https instrumentations used to have an inter-dependency, which was issue-prone (due to versioning?), so generally it’s not desirable to introduce hard dependencies between instrumentations again. (Though new optional peer dependencies might be an avenue to explore)

From the end-use perspective, it can be confusing to add an instrumentation (such as express) without http, and see nothing. Meta-packages that include a pre-defined list of instrumentations can help address that use case, basically offering an easy get started button. There’s a desire to keep the lower level SDK/instrumentations granular.

github-actions[bot] commented 2 years ago

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

github-actions[bot] commented 2 years ago

This issue was closed because it has been stale for 14 days with no activity.

rauno56 commented 2 years ago

Still an issue imho and relevant, but I don't really now a way to enforce that or take the discussion further.