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

No-op span ID generation #159

Closed
reyang opened this issue Sep 20, 2019 · 12 comments
Closed

No-op span ID generation #159

reyang opened this issue Sep 20, 2019 · 12 comments
Assignees
Labels
api Affects the API package. discussion Issue or PR that needs/is extended discussion. release:required-for-ga To be resolved before GA release tracing

Comments

@reyang
Copy link
Member

reyang commented Sep 20, 2019

  1. If the span is not sampled, we shouldn't create a real span.
  2. If the span is sampled, and there is just the API package (not the SDK package), the outgoing span (SpanKind = Server) should generate a new span id (but not a new trace id) in order to be compliant with W3C TraceContext. All the in-process spans should be dummy spans (zero span id).
  3. If the span is sampled, and there is SDK package, everything should just work, and compliant with W3C TraceContext.
  4. Integrations should not try to tell if SDK exists or not, integrations should not try to tell if SDK exists or not and perform special operations.
@Oberon00
Copy link
Member

Oberon00 commented Sep 23, 2019

I think we should just forwad the TraceContext headers as-is (if at all). I think the specification backs me up:

In https://www.w3.org/TR/trace-context/#design-overview:

Tracing tools can provide two levels of compliant behavior interacting with trace context:

  • At a minimum they MUST propagate the traceparent and tracestate headers and guarantee traces are not broken. This behavior is also referred to as forwarding a trace.
  • In addition they CAN also choose to participate in a trace by modifying the traceparent header [...]

A tracing tool can choose to change this behavior for each individual request to a component it is monitoring.

The more specific wording is is in https://www.w3.org/TR/trace-context/#mutating-the-traceparent-field:

A vendor receiving a traceparent request header MUST send it to outgoing requests. It MAY mutate the value of this header before passing it to outgoing requests.

[...] Unmodified header propagation is typically implemented in pass-through services like proxies. This behavior may also be implemented in a service which currently does not collect distributed tracing information.

(bold & cursive added by me)

Apart from the spec, I don't see the added value in deliberately going out of our way to break the parent-child chain in traces.

@reyang
Copy link
Member Author

reyang commented Sep 23, 2019

Not sure if the pass-through behavior was intended ONLY for proxy scenario (where one incoming call results in one outgoing call), or if this applies to a normal service scenario:

If service A calls B using traceparent: 00-12345678901234567890123456789012-1234567890123456-01, and B calls service C twice. Do we expect both calls to service C are using the same span id?

@Oberon00
Copy link
Member

That's the consequence: They have the same trace-parent P. Basically the whole uninstrumented service U reuses the node of the parent and one can no longer distinguish whether some service C was called directly from P or through U.
But the same happens naturally in-process, when you consider instrumenting some previously uninstrumented method U that is called by P and that calls service or method C.

@Oberon00
Copy link
Member

Oberon00 commented Sep 24, 2019

It just came to my mind that with sampling we potentially have the very same behavior when some intermediate span is not sampled. EDIT: Or do we break the trace in these cases? I'm not up to speed with the sampling spec.

@Oberon00 Oberon00 added api Affects the API package. discussion Issue or PR that needs/is extended discussion. tracing labels Sep 24, 2019
@reyang
Copy link
Member Author

reyang commented Sep 25, 2019

That's the consequence: They have the same trace-parent P. Basically the whole uninstrumented service U reuses the node of the parent and one can no longer distinguish whether some service C was called directly from P or through U.

I can see one difference - if I see service C being called several times using the same trace-parent, I know it must have to do with some broken thing between P and C.

@Oberon00
Copy link
Member

In practice, usually that would indeed indicate that something is broken. But in theory, you can do the following (pseudo-python because I'm lazy):

with tracer.start_span('call hello') as span:
 headers = dict()
 propagators.inject(span.get_context(), headers, headers.__setitem__)

 myuninstrumentedhttplib.get('http://example.com/hello', headers)
 myuninstrumentedhttplib.get('http://example.org/hello', headers) # Call another service with the same parent

@c24t
Copy link
Member

c24t commented Oct 31, 2019

Blocking on some decisions in spec, W3C. In particular whether/how the context is propagated in the API package.

@c24t c24t modified the milestones: Alpha v0.2, Alpha v0.3 Oct 31, 2019
@c24t c24t modified the milestones: Alpha v0.3, Alpha v0.4 Dec 5, 2019
@toumorokoshi toumorokoshi removed this from the Alpha v0.4 milestone Feb 27, 2020
@c24t
Copy link
Member

c24t commented Mar 5, 2020

Is this defined by the spec yet? The behavior we've chosen now is that extensions create a new (no-op in the case of no SDK) span.

@codeboten codeboten added the release:required-for-ga To be resolved before GA release label Jul 22, 2020
@codeboten codeboten assigned codeboten and unassigned c24t Aug 27, 2020
@codeboten
Copy link
Contributor

I'll follow up on this issue to find the spec issue.

@lzchen
Copy link
Contributor

lzchen commented Sep 24, 2020

  1. We already do this.
  2. Generating a new span id is incorrect, the traceid and spanid must be both invalid if absence of sdk. TODO: Validate whether the spancontext is propagated properly in the absence of SDK.
  3. We already do this.
  4. Instrumentations do not rely on the sdk, we already do this.

@codeboten
Copy link
Contributor

2\. TODO: Validate whether the spancontext is propagated properly in the absence of SDK.

Our existing propagation unit tests covers this scenario here:

self.assertEqual(traceparent_value, output["traceparent"])
self.assertIn("key3=val3", output["otcorrelations"])
self.assertIn("key4=val4", output["otcorrelations"])
self.assertIn("foo=1", output["tracestate"])
self.assertIn("bar=2", output["tracestate"])
self.assertIn("baz=3", output["tracestate"])

srikanthccv pushed a commit to srikanthccv/opentelemetry-python that referenced this issue Nov 1, 2020
Closes open-telemetry#159

Signed-off-by: Olivier Albertini <olivier.albertini@montreal.ca>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Affects the API package. discussion Issue or PR that needs/is extended discussion. release:required-for-ga To be resolved before GA release tracing
Projects
None yet
Development

No branches or pull requests

6 participants