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] service.md: normalize link to process.md #883

Merged
merged 5 commits into from
Apr 5, 2024

Conversation

chalin
Copy link
Contributor

@chalin chalin commented Apr 4, 2024

The link needs to be normalized (to refer to a page, not the page title), otherwise OTel.io link checking fails; for details, see open-telemetry/opentelemetry.io#4253 (review).

/cc @joaopgrassi

This PR normalizes the link to process.md, droping the hash to the page title, since that breaks link checking in the OTel website, see open-telemetry/opentelemetry.io#4253 (review).
@chalin chalin requested review from a team April 4, 2024 23:57
@lmolkova
Copy link
Contributor

lmolkova commented Apr 5, 2024

@chalin thank you!
what's wrong with https://github.com/open-telemetry/semantic-conventions/blob/main/docs/attributes-registry/process.md#process? why it not ok to refer to the title? Is there something we need to change to avoid things like this?

@joaopgrassi joaopgrassi added the Skip Changelog Label to skip the changelog check label Apr 5, 2024
@trisch-me
Copy link
Contributor

@lmolkova it refers to /attributes-registry/process/#process, it's probably a bug for conversion from process.md#process to /process/#process, I think the right way would be to fix instrumentation which does conversion

But the process.md#process doesn't have much sense as well as there is only process now. Maybe it's a leftover from old structure before registry

@lmolkova
Copy link
Contributor

lmolkova commented Apr 5, 2024

@lmolkova it refers to /attributes-registry/process/#process, it's probably a bug for conversion from process.md#process to /process/#process, I think the right way would be to fix instrumentation which does conversion

thanks for the clarification @trisch-me. I believe this conversion to /attributes-registry/process/#process is not part of semconv tooling and hence we can't fix it or prevent things like this in the future.

@lmolkova lmolkova merged commit 14c8e52 into open-telemetry:main Apr 5, 2024
11 checks passed
@chalin chalin deleted the patch-1 branch April 5, 2024 19:11
@chalin
Copy link
Contributor Author

chalin commented Apr 5, 2024

... what's wrong with https://github.com/open-telemetry/semantic-conventions/blob/main/docs/attributes-registry/process.md#process? why it not ok to refer to the title?

Because on the OTel.io website, page titles don't have anchors, and hence they can't be linked to. In this repo (and any other OTel spec repo), it is a bit moot to link to the title, since the title is the first thing displayed on the page. So the win-win strategy is to instead just link to the page. Do you agree?

@lmolkova
Copy link
Contributor

lmolkova commented Apr 5, 2024

@chalin I do agree, the problem is that it's hard to catch inside semconv - process.md#process is a valid link within the repo and does not fail the CI. So we have to rely on the reviewers to catch it, i.e. can't guarantee that new versions of semconv will be compatible with website links.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip Changelog Label to skip the changelog check
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

7 participants