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

Revert #1043 (Third Party Propagators location) #1047

Open
carlosalberto opened this issue Oct 1, 2020 · 13 comments
Open

Revert #1043 (Third Party Propagators location) #1047

carlosalberto opened this issue Oct 1, 2020 · 13 comments
Assignees
Labels
area:miscellaneous For issues that don't match any other area label priority:p2 Medium priority level release:allowed-for-ga Editorial changes that can still be added before GA since they don't require action by SIGs spec:context Related to the specification/context directory

Comments

@carlosalberto
Copy link
Contributor

We had a previous issue (and a fair amount of time spent discussing it - taking a pair of weeks) that ended up in the decision (with the help of a the language maintainers) to NOT maintain third party propagators as part of OpenTelemetry.

#1043 is NOT trivial as it has a related solved issue/PR and needs to be reverted - then we may discuss this again and decide if/why we really need this.

cc @dyladan
cc @SergeyKanzhelev

@carlosalberto carlosalberto added the spec:context Related to the specification/context directory label Oct 1, 2020
@SergeyKanzhelev
Copy link
Member

I read the issue and PR and I didn't get an impression there is a controversy. Perhaps it was discussed on the meeting and not reflected on the issue. Can you summarize the controversy here? How the "encouraged" language is different from "can"?

@carlosalberto
Copy link
Contributor Author

@SergeyKanzhelev This was an intermediate decision (as I said, this decision took weeks and weeks). Yes, it was definitely discussed one more time yet after and that's why we ended up changing that initial agreement that @mtwo mentioned (the discussion/changes can show that).

The controversy is that OTel maintainers do not want to maintain third party propagators and this resolution took a little while to reach.

How the "encouraged" language is different from "can"?

IMHO this will open a potential struggle between Maintainers and third parties when it comes to where to host the propagators (why yes here, why no there). I'd prefer to have the things clear.

In any case, if Maintainers decide this change is fine, I'm all up for it, but let's do it in a proper way and discuss this again with the Maintainers.

@SergeyKanzhelev
Copy link
Member

I share your worries. I said on PR it's uncontroversial and merged as the PR didn't change anything semantically - encourage language changed to can.

If the decisions would be to evict those from SDK, the change would be with the same reasoning I added mention that propagators needs to be self contained so people can use it with SDK without the need to import entire vendor's set of packages like exporters.

So if discussions are still ongoing, it will be a semantical change to what was written, but additive to what I have added. Is it something will be discussed next Tuesday meeting?

@carlosalberto
Copy link
Contributor Author

@SergeyKanzhelev Let's definitely discuss this next week. Meanwhile, I will keep this issue open.

@SergeyKanzhelev
Copy link
Member

thinking about it - maybe maintainers meeting would be even more appropriate for this. Understanding the balance between what maintainers willing to support and how easy it is for end users to consume.

@carlosalberto
Copy link
Contributor Author

SGTM

@andrewhsu andrewhsu added release:allowed-for-ga Editorial changes that can still be added before GA since they don't require action by SIGs area:miscellaneous For issues that don't match any other area label labels Oct 2, 2020
@mtwo
Copy link
Member

mtwo commented Oct 5, 2020

Following up from the maintainers meeting. There are three types of propagators:

Fully open standards

  • W3C
  • B3

Platform-specific standards

  • AWS X-Ray header
  • Google Cloud-Trace-Context (being deprecated)

Proprietary headers

  • Dynatrace
  • New Relic
  • Etc.

It is clear that fully open standards should be in the main SDK packages / repos, and that proprietary headers should not be. This discussion is about whether we should place platform-specific standards in the main repositories or contrib / external repositories.

From the discussion at the maintainers meeting, we will not put platform-specific context formats in the primary SDK repositories. They can exist within official contrib repositories, vendor-specific repositories, or anywhere else.

@mtwo
Copy link
Member

mtwo commented Oct 5, 2020

@alolita will take the action to formalize this

@SergeyKanzhelev
Copy link
Member

primary SDK repositories

Does primary mean SDK or it also includes -contrib?

@mtwo
Copy link
Member

mtwo commented Oct 5, 2020

primary SDK repositories

Does primary mean SDK or it also includes -contrib?

Just added this to clarify: They can exist within official contrib repositories, vendor-specific repositories, or anywhere else.

@andrewhsu andrewhsu added this to Required for GA, needs action. Add label:release:required-for-ga to issues and PRs when moving them to this column. in GA Spec Burndown Oct 6, 2020
@andrewhsu
Copy link
Member

from the issue triage mtg today, decision on this issue will affect go sig because imports strongly tied to repos.

@alolita talked about this and your name came up. would you be able to provide a draft for this issue by monday? quick action on this will give time for go sig to adjust. else @mtwo will do this on monday.

@carlosalberto
Copy link
Contributor Author

FYI, I will prepare a PR to revert the PR (while keeping @SergeyKanzhelev's improvements) and then @alolita will be able to provide the follow up on the Platform Specific Propagators ;)

@MrAlias
Copy link
Contributor

MrAlias commented Feb 12, 2021

Is this still happening? Moving the location of where our propagators live in the Go repos will be a breaking change. If want to do this can we resolve this soon?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:miscellaneous For issues that don't match any other area label priority:p2 Medium priority level release:allowed-for-ga Editorial changes that can still be added before GA since they don't require action by SIGs spec:context Related to the specification/context directory
Projects
No open projects
GA Spec Burndown
  
Required/Allowed for GA, todo.
Development

No branches or pull requests

6 participants