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

sdk-common: add InstrumentationScope #354

Merged
merged 1 commit into from
Nov 9, 2023

Conversation

iRevive
Copy link
Contributor

@iRevive iRevive commented Nov 5, 2023

Reference Link
Spec https://opentelemetry.io/docs/specs/otel/glossary/#instrumentation-scope
https://opentelemetry.io/docs/specs/otel/trace/api/#get-a-tracer
Java implementation InstrumentationScopeInfo.java

I started decoupling #325 into smaller changes to simplify the review process.

@iRevive iRevive added tracing Improvements to tracing module module:sdk Features and improvements to the sdk module labels Nov 5, 2023
@iRevive iRevive added this to the 0.4 milestone Nov 5, 2023
@iRevive iRevive changed the title trace sdk: add InstrumentationScope sdk-common: add InstrumentationScope Nov 5, 2023
* @see
* [[https://opentelemetry.io/docs/specs/otel/glossary/#instrumentation-scope Instrumentation Scope spec]]
*/
sealed trait InstrumentationScope {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the Java SDK calls this InstrumentationScopeInfo, any commentary?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The specification doesn't require this particular name.
Java SDK uses InstrumentationScopeInfo, Go stands with Scope, Python uses InstrumentationInfo and Rust uses InstrumentationLibrary.

From what I see, InstrumentationScope is sufficient enough and not verbose.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Python uses InstrumentationInfo

Wait a minute: Python has both an InstrumentationInfo and an InstrumentationScope. What's the difference? 🤔

Copy link
Contributor Author

@iRevive iRevive Nov 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spec-wise, perhaps they refer to InstrumentationLibrary and InstrumentationScope?

In JavaScript, there are both InstrumentationLibrary and InstrumetnationScope. InstrumentationLibrary is deprecated, though.

Comment on lines 148 to 160
private final case class ScopeImpl(
name: String,
version: Option[String],
schemaUrl: Option[String],
attributes: Attributes
) extends InstrumentationScope

private final case class BuilderImpl(
name: String,
version: Option[String],
schemaUrl: Option[String],
attributes: Attributes
) extends Builder {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually these can be combined into a single class, then we save an allocation in the build method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see reasons why we shouldn't :)

* @param version
* the version to set
*/
def setVersion(version: String): Builder
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want to be bikeshedding too much, it's not a big deal. And I guess otel4s already has some established idioms. But typically I associate set with mutability and e.g. withVersion would be more appropriate for an immutable builder like this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Valid point. We primarily use with* across the otel4s codebase.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the code

@NthPortal
Copy link
Contributor

should this be in core-common and used by TracerBuilder? also, to continue what Arman said, should setAttributes be named withAttributes?

bikeshedding that more: currently it only supports replacing the attributes. should we support adding additional attributes as well? instead? we could have withAdditionalAttributes as well. or, we could have withAttributes take a function Attributes => Attributes so that you could either entirely replace the attributes or add additional ones

@iRevive
Copy link
Contributor Author

iRevive commented Nov 8, 2023

should this be in core-common and used by TracerBuilder?

I don't think so. InstrumentationScope is primarily used by the internals: span processors, exporters, etc. End users shouldn't use it directly.

For example, tracer builder public API does not require InstrumentationScope: https://opentelemetry.io/docs/specs/otel/trace/api/#get-a-tracer. But most of the implementation will construct one under the hood.

should setAttributes be named withAttributes?

Definitely. I forgot to rename it.

we could have withAdditionalAttributes as well

I didn't find such a use case (at least yet). Since it is supposed to be used by internals, we can always add it in the future.

@iRevive iRevive force-pushed the sdk-trace/instrumentation-scope branch from 10eb7fe to b842e24 Compare November 8, 2023 19:39
@iRevive iRevive merged commit 9aff477 into typelevel:main Nov 9, 2023
10 checks passed
@iRevive iRevive deleted the sdk-trace/instrumentation-scope branch November 9, 2023 07:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module:sdk Features and improvements to the sdk module tracing Improvements to tracing module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants