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

[editorial] Use path, not external URL, for link to spec page; and fix broken link #3310

Conversation

chalin
Copy link
Contributor

@chalin chalin commented Mar 10, 2023

@chalin chalin requested review from a team March 10, 2023 00:52
@chalin chalin changed the title [editorial] Use path not URL for link to spec page [editorial] Use path, not URL, for link to spec page Mar 10, 2023
@chalin chalin changed the title [editorial] Use path, not URL, for link to spec page [editorial] Use path, not external URL, for link to spec page Mar 10, 2023
@reyang reyang added the editorial Editorial changes only (typos, changelog, ...). No content-related changes of any kind. label Mar 10, 2023
@chalin chalin marked this pull request as draft March 10, 2023 01:06
@chalin
Copy link
Contributor Author

chalin commented Mar 10, 2023

Since the following PR:

The link to "Metric Metadata" has moved. I've added the fix to this PR (through the second commit).

@chalin chalin marked this pull request as ready for review March 10, 2023 01:15
@chalin
Copy link
Contributor Author

chalin commented Mar 10, 2023

Maybe there should be a check (in this repo) to catch external links to local spec pages?

@chalin
Copy link
Contributor Author

chalin commented Mar 10, 2023

We'll be checking for this on the website side of things (FYI):

@Oberon00
Copy link
Member

Not sure how helpful that is, since the path starts with / which makes it still quite inflexible.

@chalin
Copy link
Contributor Author

chalin commented Mar 10, 2023

Hi @Oberon00. I'm not sure I understand your comment. Are you against using a path vs a full URL for local page references (because I noticed that you didn't give your approval for this PR)? Or are you saying that you'd prefer that the path be relative rather than absolute. Which extra flexibility are you looking for?

@chalin
Copy link
Contributor Author

chalin commented Mar 10, 2023

To clarify: there's an extra cost in checking external links, managing the external link caches etc., so it doesn't make sense to use an external URL for internal paths.

By the way, we do link checking (internal and external) for the entire OTel website, which includes the full specification, in 9 seconds -- for example see this build log:

7:49:24 PM: tmp/bin/htmltest --log-level 1
7:49:24 PM: htmltest started at 12:49:24 on public
7:49:24 PM: ========================================================================
7:49:33 PM: ✔✔✔ passed in 8.958333952s
7:49:33 PM: tested 347 documents

That's in contrast to the 180 seconds required by markdown-link-check running over only the spec. So it's worth paying careful attention to how links are encoded.

I hope this helps clarify the motivation behind this PR. Thanks!

@chalin chalin changed the title [editorial] Use path, not external URL, for link to spec page [editorial] Use path, not external URL, for link to spec page; and fix broken link Mar 10, 2023
@Oberon00
Copy link
Member

because I noticed that you didn't give your approval for this PR

Sorry for misunderstandings, this was just a curious comment. I didn't mean to indicate disapproval (I haven't reviewed the PR just because I don't have a qualified opinion on it, and I don't review every PR I comment on 🙂)

@reyang reyang merged commit 91baf54 into open-telemetry:main Mar 10, 2023
@chalin
Copy link
Contributor Author

chalin commented Mar 10, 2023

@Oberon00 - no worries, thanks for clarifying. Thanks all for the review and merging.

@chalin chalin deleted the chalin-im-use-local-path-into-spec-2023-03-09 branch March 10, 2023 15:57
carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this pull request Oct 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
editorial Editorial changes only (typos, changelog, ...). No content-related changes of any kind.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants