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

Should there be a "prepared span" construct in the API? #734

Open
yurishkuro opened this issue Jul 24, 2020 · 9 comments
Open

Should there be a "prepared span" construct in the API? #734

yurishkuro opened this issue Jul 24, 2020 · 9 comments
Labels
area:api Cross language API specification issue release:after-ga Not required before GA release, and not going to work on before GA spec:trace Related to the specification/trace directory

Comments

@yurishkuro
Copy link
Member

Many span attributes are "static", i.e. they never change for the given class of spans. Span name is one such attribute, but there are many others from semantic conventions, e.g. http.method or db.type. Right now the trace API does not provide any facilities for optimization, requiring setting all those attributes on the spans over and over, on the critical path of the requests.

A possible solution to this is the concept of prepared span, somewhat similar to "bound instrument" in the metrics API, or "prepared statement" in the database APIs, e.g.

func NewDBMiddleware(traceProvider, dbDriver) *Middleware {
    tracer := traceProvider.GetTracer("me, the middleware")
    return &Middleware{
        preparedSpan: tracer.PrepareSpan(
          "db-query", 
          Attribute{"db.type", "MySQL"},
          Attribute{"db.connection", dbDriver.Connection.String()},
       )
    }
}

func (m *Middleware) Execute(query, next) {
    span := m.preparedSpan.Start()
    span.SetAttribute("db.statement", query)
    defer span.FinishI()

    next(query)
}

Preparing the span allows the SDK to optimize how the span is stored in memory and exported to the backend, e.g. static attributes can be either encoded more efficiently or not sent with each span at all.

@Oberon00
Copy link
Member

Oberon00 commented Jul 24, 2020

Note that we had (still have) the same problem with a hypothetical http.app attribute (#335) where @SergeyKanzhelev also noted that it would be the same most of the time in #263 (comment). Which led me to create #274 to allow resources as span attributes. This would solve the same problem, but instead of preparing a span and then finishing it, you would prepare a resource and then set it as a an attribute of the span.

@jmacd has a more complex proposal to solve this problem at open-telemetry/oteps#78. @tigrannajaryan also suggested there should be some way to have more than one resource per span in #579.

@Oberon00
Copy link
Member

@open-telemetry/specs-approvers @open-telemetry/specs-trace-approvers Should we create some label like "shared-span-data" to track all these issues?

@bogdandrutu
Copy link
Member

When I read this I immediately thought about the Builder being possible to be reused (because it is kind of that intermediate state). Not sure if that is the right approach but would like us to think about the relationship between this proposal and the current Builder (in some languages that use this pattern).

@yurishkuro
Copy link
Member Author

@bogdandrutu I avoided calling it a "builder" because builders are usually not designed to be reusable. In languages that already use builders the above example would change to preparedSpan.builder().(other setters).build().

@bogdandrutu bogdandrutu added area:api Cross language API specification issue release:required-for-ga Must be resolved before GA release, or nice to have before GA spec:trace Related to the specification/trace directory labels Jul 24, 2020
@bogdandrutu
Copy link
Member

Marked this as required for GA to make a decision if this is important enough or not to have it before GA.

@bogdandrutu bogdandrutu added the priority:p2 Medium priority level label Jul 24, 2020
@jmacd
Copy link
Contributor

jmacd commented Jul 24, 2020

Although we haven't discussed this concept much before, it's been on my mind. The proposal in open-telemetry/oteps#78 was about API support for preparing a Tracer with static resources, and the issue raised here is more about static properties of the Span. One example that comes to mind is that the relationship between a Span Name and Span Kind ought to be fixed, and I'd like if the API made it possible to declare a span name and kind once, not allow me to vary these with respect to each other at runtime.

This also made me think through the difference between Metrics and Tracing API in this regard--why do we require a metric instrument definition? It's both about performance and about ensuring a consistent definition, i.e., making it difficult to mix metric instrument kinds in the same process and at the same time improving performance. I would definitely support some kind of prepared Span API and see it as complimentary to the resource-scope proposal in OTEP issue 78.

Now a slightly tangential remark. I've thought through the above observation about making a "Span instrument", which would be a prepared span in the sense proposed here by @yurishkuro and similar in spirit to a Metric instrument. The reason I like thinking through this is that the Metrics SDK by design is able to flush state about metric label sets that haven't been used over several collection intervals. Today's conventional Tracing SDK by design allows you to leave a Span open forever and has few tools to protect the user against mis-use.

If we had a Span instrument, it could be implemented based on the Metric SDK (with a new "SpanAggregator" that forms SpanData by aggregating span events) and we could avoid the problem with leaked spans. Whereas today's metric SDK keeps a record for each Metric instrument and label set with pending updates, the metric SDK would (with simple modifications, I believe) keep a record for every active Span (i.e., combination of Span instrument and trace_id/span_id label set); this would enable a timeout to be set, allowing the SDK to eject spans from memory if they are left open too long, which I think would be a huge win for OTel users.

@jmacd
Copy link
Contributor

jmacd commented Jul 24, 2020

An even-more tangential remark is that by using the Metric SDK to index SpanInstrument/trace_id/span_id as the primary data store, the problem discussed in open-telemetry/oteps#73 could be solved easily.

@tigrannajaryan
Copy link
Member

This indeed can be very useful for extracting more performance. The Protobufs naturally lend to such optimization and OTLP exporter could serialize prepared attributes once (e.g. like this).

Nevertheless I think this is an additive API and is not a breaking change to existing APIs. Should this be a release:required-for-ga or we can remove the label?

@Oberon00 Oberon00 added release:after-ga Not required before GA release, and not going to work on before GA and removed release:required-for-ga Must be resolved before GA release, or nice to have before GA labels Sep 9, 2020
@andrewhsu andrewhsu removed the priority:p2 Medium priority level label Sep 9, 2020
@jmacd
Copy link
Contributor

jmacd commented Oct 4, 2022

@lquerel has pointed out that a prepared span (a.k.a. "Span Instrument") would help SDKs lower the cost of column-oriented encoding and transport.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:api Cross language API specification issue release:after-ga Not required before GA release, and not going to work on before GA spec:trace Related to the specification/trace directory
Projects
None yet
Development

No branches or pull requests

6 participants