API prototype #11
API prototype #11
Conversation
api/lib/opentelemetry/trace/span.rb
Outdated
|
|
||
| def add_link(span_context_or_link, **attrs) | ||
| check_not_nil(span_context_or_link, "span_context_or_link") | ||
| check_empty(attrs, "attrs") unless span_context_or_link.instance_of?(SpanContext) |
There was a problem hiding this comment.
The instance_of? is OK here because SpanContext is specified to be a "final" class and add_link is specified to either take a SpanContext + attributes or a Link without additional attributes.
|
I was reading through the specification in order to understand exactly what is expected of the API portion of this project. Bullet point #2 under the requirements section says:
From this, I gather that an instrumentation author should only need to depend on the opentelemety-ruby-api gem and be able to write code such as: In order for this to work, the API will have to ship with a no-op tracer, since a valid, usable span will need to be returned by Tracer#create_span. This is how the API is packaged for OpenTracing. See https://github.com/opentracing/opentracing-ruby and the tracer. Since Ruby doesn't have a notion of interfaces, it's not clear if there should be an additional set of empty classes showing the expected method signatures. For OpenTracing the no-op Tracer is all that was packaged with the API library. I'm wondering if it's possible for us to do the same for this project's API. I took a look through several of the OpenTelemetry libraries in order to see how they're structured and this is what I found: Java's API Library contains both interfaces, as well as a default no-op Tracer implementation
I looked at Python as well, and the situation there was a little bit unclear to me. Perhaps somebody can shed some light on it. Its API imports the typing module. I only see method signatures with type hints, but nothing indicating that the API returns a usable object. Which, seems like it wouldn't satisfy the requirement I listed at the beginning of this comment. I also looked at the Javascript repo. It ships with interfaces, available by way of type script, and it appears as though it will ship with a no-op tracer, given at least some of the open PRs.
Based on this, I think it is a requirement that the OpenTelemetry Ruby API ships with a no-op Tracer implementation. I'm not sure if that is sufficient, but I think it might be. Since Ruby does not have interfaces, shipping a set of empty classes does not seem valuable. I think we need to clarify what is expected, but ultimately, if packaging a no-op Tracer implementation is sufficient for the Ruby API, perhaps we should shift gears slightly, and start heading that direction. |
The requirement for a minimal implementation in the API package is spelled out below. I agree it doesn't make sense to 🚢 a set of empty classes. I've updated the PR to provide a minimal implementation instead. For alternative implementations, I think we should provide a conformance test suite alongside the API package.
|
| end | ||
|
|
||
| def start_root_span(name, sampler: nil, links: nil, recording_events: nil, kind: nil) | ||
| raise ArgumentError if name.nil? |
There was a problem hiding this comment.
I'm not sure what we should do for invalid arguments in the minimal implementation, and am somewhat torn on the issue.
I just wanted to point out that there is a proposal currently in PR form here: open-telemetry/opentelemetry-specification#153.
There seem to be various opinions as you read through the comments. This comment proposed a somewhat pragmatic approach of falling back to a default when an argument is invalid. Quote (from the comment)
Specific case: I do think using a null name for Span could fallback to using a default name, instead of throwing. Documenting it is nice, but providing defaults for these kind of things is a good alternative ;)
It might be too early to make a final call on how to deal exceptions in the minimal implementation, but we should keep an eye on that PR and the specification repo for guidance.
| module Trace | ||
| # SpanData is an immutable object that is used to report out-of-band completed spans. | ||
| # | ||
| # TODO: consider whether to copy collections to a known internal form and expose only enumerations. |
There was a problem hiding this comment.
I really like this idea. I think the no-op implementation should try to expose the smallest possible interface. That will help users ensure their usage is correct (e.g. they aren't inadvertently depending on the returned objects implementing obscure methods of Array or Hash), and relieve implementors of the burden of implementing a larger surface.
There was a problem hiding this comment.
Opened #15 for this. I don't want to invest effort until there's some clarity around SpanData (open-telemetry/opentelemetry-specification#71) and enumerable vs. hash attributes (open-telemetry/opentelemetry-specification#92).
| # TODO: This is a helper for the default use-case of extending the current trace with a span. | ||
| # | ||
| # The spec-ed API seems a little clunky. Default use-case looks like: | ||
| # OpenTelemetry.tracer.with_span(OpenTelemetry.tracer.start_span('do-the-thing')) do ... end |
There was a problem hiding this comment.
I like this helper 👍
The spec assumes Java -- looks like in the reference impl, scope implements Closeable interface and used along with try-with-resources.. We don't need to follow that pattern.
Co-Authored-By: Yoshinori Kawasaki <luvtechno@users.noreply.github.com>
|
Updated with review suggestions. This needs a 👍 from a codeowner to merge. |
|
Oh quick nitpick: might want to change PR title before merging, just to keep things clean. |
This is an early prototype of the OpenTelemetry Ruby API based on the API spec so far. Very much WIP, but feedback and discussion about any of this is strongly encouraged.