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.

Sign up for GitHub

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

Jump to bottom

Move OTLP specification to github.com/open-telemetry/opentelemetry-proto #3454

Merged
merged 1 commit into from
May 24, 2023
Merged

Move OTLP specification to github.com/open-telemetry/opentelemetry-proto #3454

merged 1 commit into from
May 24, 2023

Conversation

tigrannajaryan
Copy link
Member

@tigrannajaryan tigrannajaryan commented Apr 27, 2023 •
edited
Loading

@tigrannajaryan tigrannajaryan requested review from a team April 27, 2023 19:19
@tigrannajaryan
Copy link
Member Author

This will fail until open-telemetry/opentelemetry-proto#458 is merged.

jsuereth
jsuereth approved these changes Apr 27, 2023
View reviewed changes
Copy link
Contributor

@jsuereth jsuereth left a comment

Choose a reason for hiding this comment

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

Approved, pending acceptance in proto repository.

@tigrannajaryan
Copy link
Member Author

Don't merge until open-telemetry/opentelemetry.io#2642 is resolved.

@github-actions
Copy link

github-actions bot commented May 6, 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 6, 2023
chalin
chalin reviewed May 8, 2023
View reviewed changes
Copy link
Contributor

@chalin chalin left a comment

Choose a reason for hiding this comment

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

Don't merge until open-telemetry/opentelemetry.io#2642 is resolved.

FYI, since this PR isn't deleting any pages, there is no need to wait on open-telemetry/opentelemetry.io#2642.

If later you decide to delete the pages (though not the section), we can add aliases to the updated section page to avoid any 404s on the website.

/cc @svrnm @cartermp

@tigrannajaryan
Copy link
Member Author

I think we still need to wait for open-telemetry/opentelemetry.io#2642
If we release 1.21 the spec pages will be automatically updated.

@arminru arminru added the spec:protocol Related to the specification/protocol directory label May 8, 2023
arminru
arminru approved these changes May 8, 2023
View reviewed changes
CHANGELOG.md Show resolved Hide resolved
@github-actions github-actions bot removed the Stale label May 9, 2023
chalin
chalin reviewed May 9, 2023
View reviewed changes
Copy link
Contributor

@chalin chalin left a comment •
edited
Loading

Choose a reason for hiding this comment

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

See inline comments for copyedits.

Also note the potential impact of the following proposal:

/cc @svrnm @cartermp

specification/protocol/README.md Outdated Show resolved Hide resolved
specification/protocol/README.md Outdated Show resolved Hide resolved
specification/protocol/README.md Outdated Show resolved Hide resolved
specification/protocol/design-goals.md Outdated Show resolved Hide resolved
specification/protocol/design-goals.md Outdated Show resolved Hide resolved
specification/protocol/requirements.md Outdated Show resolved Hide resolved
specification/protocol/otlp.md Outdated Show resolved Hide resolved
@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 20, 2023
@chalin
Copy link
Contributor

chalin commented May 24, 2023

@tigrannajaryan et al.: is anything holding up this PR?

@tigrannajaryan
Copy link
Member Author

@tigrannajaryan et al.: is anything holding up this PR?

I was waiting for the completion of open-telemetry/opentelemetry.io#2642 to make sure we don't break anything by deleting the content here.

@chalin
Copy link
Contributor

chalin commented May 24, 2023

If you rebase this branch, I can test the branch, but the simplest would be to just merge in the changes, and then I'll test from main and submit any necessary followup changes (if any). WDYT @tigrannajaryan?

@tigrannajaryan tigrannajaryan force-pushed the feature/tigan/moveotlp branch 6 times, most recently from f84d94b to c717ebe Compare May 24, 2023 18:37
@tigrannajaryan tigrannajaryan force-pushed the feature/tigan/moveotlp branch from c717ebe to 6a684b6 Compare May 24, 2023 18:47
@chalin
Copy link
Contributor

chalin commented May 24, 2023

Maybe someone should remove the Stale label?

@tigrannajaryan
Copy link
Member Author

If you rebase this branch, I can test the branch, but the simplest would be to just merge in the changes, and then I'll test from main and submit any necessary followup changes (if any). WDYT @tigrannajaryan?

This is rebased and ready to merge.

@chalin let me if you need to confirm anything on your end, otherwise I can merge. I assume merging this changes nothing on our live website at least not until the next release.

@chalin
Copy link
Contributor

chalin commented May 24, 2023

@tigrannajaryan - I just tested this PR and it breaks the website in obvious ways (detailed below) that will be fixed once the OTLP spec is published on the website (eg., via open-telemetry/opentelemetry.io#2786), and the next version of this OTel spec gets published too.

So from my perspective, this PR is good to go. 🚀

This breaks the OTel website:

  • Shortcode can no longer extract the OTLP status
  • hash does not exist --- docs/instrumentation/python/exporters/index.html --> /docs/specs/otel/protocol/otlp/#otlphttp

@tigrannajaryan
Copy link
Member Author

@tigrannajaryan - I just tested this PR and it breaks the website in obvious ways (detailed below) that will be fixed once the OTLP spec is published on the website (eg., via open-telemetry/opentelemetry.io#2786), and the next version of this OTel spec gets published too.

So from my perspective, this PR is good to go. 🚀

This breaks the OTel website:

  • Shortcode can no longer extract the OTLP status
  • hash does not exist --- docs/instrumentation/python/exporters/index.html --> /docs/specs/otel/protocol/otlp/#otlphttp

This doesn't really break the website, right? There will be no immediate changes to https://opentelemetry.io/ if we merge this PR. All the files modified by this PR are just in this repo. The website itself hosts its own files.

Do I misunderstand something?

@chalin
Copy link
Contributor

chalin commented May 24, 2023

Apologies for the confusion. What I meant to say is that in a local build, this PR breaks the website build.

This doesn't really break the website, right? There will be no immediate changes to https://opentelemetry.io/ if we merge this PR. All the files modified by this PR are just in this repo. The website itself hosts its own files.

Correct, it won't break the production build because the OTel website is pinned at v1.21.0 of the spec.

@tigrannajaryan tigrannajaryan merged commit c317665 into open-telemetry:main May 24, 2023
@tigrannajaryan tigrannajaryan deleted the feature/tigan/moveotlp branch May 24, 2023 22:58
This was referenced May 25, 2023
Open
Merged
pellared added a commit to pellared/opentelemetry-specification that referenced this pull request Jun 2, 2023
@pellared
Fix hyperlink for open-telemetry#3454 in CHANGELOG.md
f79431b
reyang pushed a commit that referenced this pull request Jun 2, 2023
@pellared
Fix hyperlink for #3454 in CHANGELOG.md (#3531)
6941cad
@pellared pellared mentioned this pull request Jun 6, 2023
Merged
codeboten pushed a commit to open-telemetry/opentelemetry-collector that referenced this pull request Jun 6, 2023
@pellared
[chore] Update OTLP spec URL (#7829)
897a316 
Follows open-telemetry/opentelemetry-specification#3454
carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this pull request Oct 31, 2024
@tigrannajaryan
Move OTLP specification to github.com/open-telemetry/opentelemetry-pr…
78fee9b 
…oto (open-telemetry#3454)

Resolves
open-telemetry#3415

Depends on
open-telemetry/opentelemetry-proto#458
Depends on
open-telemetry/opentelemetry.io#2642
carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this pull request Oct 31, 2024
@pellared
Fix hyperlink for open-telemetry#3454 in CHANGELOG.md (open-telemetry…
38d6200 
…#3531)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pellared pellared pellared left review comments

@chalin chalin chalin left review comments

@jack-berg jack-berg jack-berg approved these changes

@arminru arminru arminru approved these changes

@joaopgrassi joaopgrassi joaopgrassi approved these changes

@carlosalberto carlosalberto carlosalberto approved these changes

@jsuereth jsuereth jsuereth approved these changes

Assignees

@arminru arminru

Labels
spec:protocol Related to the specification/protocol directory
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Proposal: move OTLP spec to github.com/open-telemetry/opentelemetry-proto
8 participants
@tigrannajaryan @chalin @jsuereth @carlosalberto @pellared @joaopgrassi @arminru @jack-berg