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

Clarify specification compliance #2968

Closed
wants to merge 25 commits into from

Conversation

reyang
Copy link
Member

@reyang reyang commented Nov 18, 2022

Follow up open-telemetry/opentelemetry-go#3399 (comment) and open-telemetry/opentelemetry-go#3399 (comment).

Notes: Given the broad impact, we will leave this PR open for Dec. 2022 and Jan. 2023 to collect more perspectives.

specification/README.md Outdated Show resolved Hide resolved
@carlosalberto
Copy link
Contributor

carlosalberto commented Nov 21, 2022

Also, let's mention this in the next Maintainers & Spec calls, so everybody is very aware of this ;)

Copy link
Contributor

@ahayworth ahayworth left a comment

Choose a reason for hiding this comment

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

This feels too broad to me - I would even go so far to say it's an overreaction.

I would feel much more comfortable if the specification instead carved out an exception for "experimental" work, so that we can encourage language SIGs to try new things to bring back to the broader community. Such experimental work should be clearly delineated as not part of the official SDK and subject to change / breakage at any point. We can put very stern warnings on such "experimental" extensions.

The language here is very broad, and I'm worried that it'll have a significant chilling effect on experimentation by language groups. Oftentimes it's useful to have a working implementation of things proposed to spec groups, and we should not discount the useful benefits of having it easily installable alongside official SDKs. With sufficient warnings and segmentation, this can be safe and effective.

The proposal here will create more problems than it solves, because any SDK that attempts to still do experimental work now faces a very fuzzy test for compliance. I don't foresee this providing a lot of clarity to the group as a whole. I believe we should not adopt this, and instead the Go SDK should simply not do what they were proposing to do. Does it really need to go farther?

specification/README.md Outdated Show resolved Hide resolved
Co-authored-by: Andrew Hayworth <ahayworth@gmail.com>
@dyladan
Copy link
Member

dyladan commented Nov 29, 2022

I agree with ahayworth. By the current wording it is actually disallowed to do prototypes of unspecified features, experimental semantic conventions, etc. In the meeting before the holiday we discussed that the API may be very strict but the SDK should be allowed to do some experimentation.

This also disallows the types of "convenience API wrappers" which I know @tedsuo has been pushing for for a long time, but others also find quite valuable.

Copy link
Member

@dyladan dyladan left a comment

Choose a reason for hiding this comment

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

Requesting changes to indicate that I am not happy with the existing wording. I am not a spec approver so this is non-binding, but I am a maintainer. For details see my above comment.


* It MUST satisfy all the "MUST", "MUST NOT", "REQUIRED", "SHALL", and "SHALL
NOT" requirements defined in the [specification][].
* It MUST NOT provide additional features or functionalities that are not
Copy link
Contributor

Choose a reason for hiding this comment

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

I recommend that this section only apply to the instrumentation API.

Copy link
Contributor

@ahayworth ahayworth Nov 29, 2022

Choose a reason for hiding this comment

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

instrumentation API

By this you mean "the API" vs "the SDK", yes? "Instrumentation API" is a new term to me, and I want to make sure I understand what you mean by it. 😄

Copy link
Member

Choose a reason for hiding this comment

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

I think he's referring to "instrumentation API" to mean the API yes. All of the calls that instrumentations make into the API to actually create spans/metrics/logs. I think this does not include the helpers some languages have created for creating instrumentation infrastructure like patching code.

@MikeGoldsmith
Copy link
Member

I also think that the wording is too limiting on experimentation and developing of features outside of the spec. There are many, many things that could potentially be added to the spec in the distant future so saying they cannot be added until the spec acknowledges them could be years apart.

As others have said, there should be strict guidelines on how the API looks and what it does, but SDK features for convenience should be allowed. Otherwise we're going to have bare SDKs and promote everything else be some sort of extension which I think will lead to community confusion and frustration.

@github-actions
Copy link

github-actions bot commented Dec 9, 2022

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Dec 9, 2022
@arminru arminru removed the Stale label Dec 13, 2022
@github-actions github-actions bot added the Stale label Mar 5, 2023
@github-actions
Copy link

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions bot closed this Mar 13, 2023
@arminru arminru reopened this Mar 14, 2023
@github-actions
Copy link

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions bot closed this Mar 22, 2023
@arminru arminru reopened this Mar 22, 2023
@arminru arminru removed the Stale label Mar 22, 2023
@github-actions
Copy link

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Mar 30, 2023
@github-actions
Copy link

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions bot closed this Apr 26, 2023
@arminru arminru reopened this Apr 26, 2023
@dyladan
Copy link
Member

dyladan commented Apr 26, 2023

I think this is an improvement but still doesn't allow for convenience APIs. Is that intentional? One example that is already available in several languages is with_span.

My thinking is that the convenience APIs might be scenario based and probably fit for separate components that users can optionally choose to use. If a particular programming language has some established pattern across majority of the scenarios, then it is already a "language specific" feature to me. I can give one example which is the "Dependency Injection" in .NET - there are folks who like the DI pattern and there are folks who do not, for ASP.NET Core applications it seems DI is the recommended path.
Regarding with_span, I actually consider this as "language specific", I treat "language specific" as the opposite of "language agnostic". with_span is useful for a subset of programming languages which leverage static scope or some syntactic sugar to make the code leaner.

IMO that is a dangerously broad definition of language specific and renders the rest of the text essentially meaningless.

@reyang do you not agree?

@reyang
Copy link
Member Author

reyang commented Apr 26, 2023

@reyang do you not agree?

I disagree with the "dangerously broad definition" part, I personally feel it is okay as an intentional ambiguity.
I agree that people could have different interpretations of what "language specific" means, I don't think we're in position to formally define what is "language specific".

@arminru arminru removed the Stale label Apr 26, 2023
@github-actions
Copy link

github-actions bot commented May 4, 2023

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label May 4, 2023
@dyladan
Copy link
Member

dyladan commented May 5, 2023

If we're going to allow APIs to add helper methods specific to their langauge we should state that specifically. Also, what happens if the otel API adds a method with the same name in the future?

@github-actions github-actions bot removed the Stale label May 6, 2023
@github-actions
Copy link

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label May 14, 2023
@github-actions
Copy link

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions bot closed this May 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet