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

Decide if instrumentation libraries are allowed to emit using more than one schema #1695

Closed
tigrannajaryan opened this issue May 13, 2021 · 11 comments
Labels
spec:miscellaneous For issues that don't match any other spec label

Comments

@tigrannajaryan
Copy link
Member

Original discussion here #1666 (comment)

Should libraries be able to obtain Tracer/Meter using the same (name,version) pair but supply a different schema_url?

Should we disallow that so that it is an error to obtain a Tracer that uses the same (name,version) pair but a different schema_url? This assumes each library should choose one schema and stick to it.

Should we allow but discourage it?

Or is it perfectly fine to do so?

@tigrannajaryan tigrannajaryan added the spec:miscellaneous For issues that don't match any other spec label label May 13, 2021
@tigrannajaryan
Copy link
Member Author

@jkwatson @reyang I think I incline towards what @carlosalberto suggested:

Libraries may obtain multiple tracers/meters with different URLs.

It is fine from schema handling perspective for one library to emit some of its data in one schema, some in another schema. From the recipient perspective this is not more complicated to handle than multiple libraries emitting in different schemas.

Optionally, the API documentation may still advise against doing it simply because it is easy to make a mistake in such cases and for the library to emit telemetry in the wrong schema if the developer mixes up the Trace/Meter. No strong opinion on this one. Perhaps this is not necessary.

So essentially this means that the Tracer/Meter needs to be identified by (name,version,schema_url) tuple instead of the current (name,version) pair.

I cannot see any downsides, but perhaps I am missing something.

What do you think?

@jkwatson
Copy link
Contributor

I'm totally fine with the schema_url being a part of the identity. It just needs to be stated explicitly in the spec.

@iNikem
Copy link
Contributor

iNikem commented May 14, 2021

I cannot imagine a use-case when an instrumentation library may want to emit different schemas during the same run. But whatever is easier to implement :)

@Oberon00
Copy link
Member

There is also the issue of a single span having more than one schema. E.g. an authentication middleware might call GetCurrentSpan().SetAttribute("enduser.role", role), which corresponds to the OTel v1 semantic conventions. But it can only hope that the span it attaches the attribute to has a compatible schema URL.

Another, maybe more common, issue is with the exception event, which may get added to all kinds of spans, even ones that might use a custom schema_url.

@carlosalberto
Copy link
Contributor

carlosalberto commented May 14, 2021

Guess my fear about identifying a Tracer only by name and version is the following:

// You are getting a Tracer, but you are also implicitly setting the schema for this namne/version for-ever.
GetTracer("myapp", "1.0", mySchema01).StartSpan(...)
...
// Undesirable scenario. 
// This will actually use mySchema01. Would logging an error would be enough?
GetTracer("myapp", "1.0", mySchema02).StartSpan() 

Also, I know we are trying our best to prevent the user from doing bad things, but there's only so much we can do ;)

@tigrannajaryan
Copy link
Member Author

There is also the issue of a single span having more than one schema. E.g. an authentication middleware might call GetCurrentSpan().SetAttribute("enduser.role", role), which corresponds to the OTel v1 semantic conventions. But it can only hope that the span it attaches the attribute to has a compatible schema URL.

A single span can certainly only have attributes from one schema. One should never mix semantic conventions from different versions into one span. However, I do not see how this is related to the issue we are discussing. You cannot create one span using two different tracers.

The issue we are discussing is whether it is ok for one library to create two or more Tracers (or Meters), such that the Tracers have different schema_urls and then emit spans using each of the Tracers, with each spans obviously having attributes that match the schema_url of the Tracer it is emitted with.

I think the answer is yes, it should be possible, although probably awkward and undesirable, it does not create any issues for the readers of the spans. Each span will be emitted in the appropriate InstrumentationLibrary message (when using OTLP), having the correct schema_url.

Another, maybe more common, issue is with the exception event, which may get added to all kinds of spans, even ones that might use a custom schema_url.

Events in the spans must use the schema_url as the span. It is not possible to create an Event using Tracer1 in span that is created using Tracer2 (assuming Tracer1 and Tracer2 have different schem_urls).

@tigrannajaryan
Copy link
Member Author

Guess my fear about identifying a Tracer only by name and version is the following:

// You are getting a Tracer, but you are also implicitly setting the schema for this namne/version for-ever.
GetTracer("myapp", "1.0", mySchema01).StartSpan(...)
...
// Undesirable scenario. 
// This will actually use mySchema01. Would logging an error would be enough?
GetTracer("myapp", "1.0", mySchema02).StartSpan() 

@carlosalberto I agree with you. In your example GetTracer("myapp", "1.0", mySchema02).StartSpan() should actually use mySchema02, it should not return the first Tracer, it should return a different Tracer. So, the Tracer should be identified by (name,version,schema_url), in my opinion.

@Oberon00
Copy link
Member

it should not return the first Tracer, it should return a different Tracer

I agree. But in my opinion, a different or the same tracer instance being returned by GetTracer should totally be an implementation detail. Ideally, constructing a tracer is so cheap that maintaining a hash-table of existing tracers makes no sense. There is also no need to enumerate all existing tracers or anything like that.

@Oberon00
Copy link
Member

@tigrannajaryan

A single span can certainly only have attributes from one schema. [...]
The issue we are discussing is whether it is ok for one library to create two or more Tracers (or Meters)

I agree that this is a different issue, I opened #1700.

@tigrannajaryan
Copy link
Member Author

it should not return the first Tracer, it should return a different Tracer

I agree. But in my opinion, a different or the same tracer instance being returned by GetTracer should totally be an implementation detail. Ideally, constructing a tracer is so cheap that maintaining a hash-table of existing tracers makes no sense. There is also no need to enumerate all existing tracers or anything like that.

I agree. We are on the same page.

@reyang
Copy link
Member

reyang commented May 18, 2021

Discussed during the spec SIG, this is covered by #1666, closing for now.

@reyang reyang closed this as completed May 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spec:miscellaneous For issues that don't match any other spec label
Projects
None yet
Development

No branches or pull requests

6 participants