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

Renamed specification and semantic_conventions directories for more clarity #161

Closed

Conversation

AlexanderWert
Copy link
Member

@AlexanderWert AlexanderWert commented Jul 5, 2023

Summary of changes:

  • As proposed in this comment: getting rid of the semantic_conventions sub-dir in the resource directory
  • renamed semantic_conventions directory to model (that's the one containing the yaml files)
  • renamed specification directory to semantic_conventions (that's the one containing the .md files)
  • fixed all the cross-linking correspondingly

@Oberon00
Copy link
Member

Oberon00 commented Jul 5, 2023

(Never use the word "final" with software-related projects... #158)

I would suggest to instead rename the "specification" top-level directory to "semantic_conventions".

EDIT: Oh, that name is already taken. Maybe a different one could be found for the YAML directory, the current one has lead to confusion in the past already. Since this repository is now only about the conventions, maybe name it model, or yaml.

@AlexanderWert
Copy link
Member Author

(Never use the word "final" with software-related projects... #158)

Very true! :-D

@AlexanderWert AlexanderWert requested review from a team July 5, 2023 09:29
@AlexanderWert AlexanderWert changed the title Moved resource semconv directly under the resource directory Renamed specification and semantic_conventions directories for more clarity Jul 5, 2023
@AlexanderWert AlexanderWert force-pushed the md-restruct-resource branch 2 times, most recently from 6e819d2 to 8273e98 Compare July 5, 2023 09:35
@AlexanderWert
Copy link
Member Author

I would suggest to instead rename the "specification" top-level directory to "semantic_conventions".

EDIT: Oh, that name is already taken. Maybe a different one could be found for the YAML directory, the current one has lead to confusion in the past already. Since this repository is now only about the conventions, maybe name it model, or yaml.

@Oberon00 Pushed a commit with your proposal. This makes it a huge change (touching many files).

What do others think, should we do that change now? If not I can revert the last commit.

@arminru
Copy link
Member

arminru commented Jul 6, 2023

I would suggest to instead rename the "specification" top-level directory to "semantic_conventions".
EDIT: Oh, that name is already taken. Maybe a different one could be found for the YAML directory, the current one has lead to confusion in the past already. Since this repository is now only about the conventions, maybe name it model, or yaml.

@Oberon00 Pushed a commit with your proposal. This makes it a huge change (touching many files).

What do others think, should we do that change now? If not I can revert the last commit.

I like the idea with semantic_conventions/ for what people usually look for (the rendered conventions) and model/ or yaml/ for the backing YAML files 👍

I even found myself open the semantic_conventions/ folder and then going back and into specification/ too often when looking for the MD files. For new readers not experienced with the repo it might be even more confusing to just find hard-to-read YAMLs there. One might not think about going into specification/ at first as it sounds like it would just contain definitions on what a semantic convention is or how the syntax looks like.

@arminru
Copy link
Member

arminru commented Jul 6, 2023

If we are going to go ahead with this change I'd propose doing so asap and also before we release the first version from this repo. This way we only cause the pain and churn of breaking all the links once since incoming links will already need to be updated from the spec repo to this one anyway, then the changed paths on top don't make it much worse now but they would do so later.

@jsuereth
Copy link
Contributor

jsuereth commented Jul 6, 2023

@arminru I'm with you that we either need to do this change ASAP to avoid causing more pain or delay it until later. Originally, to avoid too much churn all at once, I wanted to delay on this aspect and just focus on refactoring the human-readable structure and website.

@chalin FYI - If this PR is adopted and merged, we'll need to (hopefully not dramatically) change the website integration at that point.

@AlexanderWert Can you split out the change As proposed https://github.com/open-telemetry/semantic-conventions/pull/158#issuecomment-1619542856: getting rid of the semantic_conventions sub-dir in the resource directory from this PR?

@joaopgrassi
Copy link
Member

@chalin will we need to rename the folder where the markdown files are to docs as done in open-telemetry/opentelemetry-proto#469?

@chalin
Copy link
Contributor

chalin commented Jul 6, 2023

@chalin will we need to rename the folder where the markdown files are to docs as done in open-telemetry/opentelemetry-proto#469?

@joaopgrassi @jsuereth - yes, I'd vote to name the docs folder docs.

@AlexanderWert
Copy link
Member Author

@jsuereth , @arminru
I created a separate PR with the getting rid of the semantic_conventions sub-dir in the resource directory change only:

@Oberon00
Copy link
Member

Oberon00 commented Jul 6, 2023

@chalin

yes, I'd vote to name the docs folder docs.

So far we have been already using semantic_conventions. I would slightly prefer that over "docs" because they are not documentation of the conventions, but the actual conventions.
(Although I'd agree that semantic_conventions is a bit long and not very nice, but it's more clear IMHO)
(Maybe we should keep "specification" and just rename the YAML folder?)

@AlexanderWert
Copy link
Member Author

@chalin

yes, I'd vote to name the docs folder docs.

So far we have been already using semantic_conventions. I would slightly prefer that over "docs" because they are not documentation of the conventions, but the actual conventions.

@Oberon00 Right now the .md files are in specification, not semantic_conventions

@chalin
Copy link
Contributor

chalin commented Jul 6, 2023

Here's my reasoning:

  • Under this semantic-conventions repo it seems ambiguous to have a folder named semantic-conventions (so renaming to model seems like a good move to me).
  • If the *.md files are the semcon specification, with all content being normatively binding, then the folder should stay named specification.
  • If the *.md files have both normative and non-normative (commentary, examples, etc) parts, IMHO it makes more sense to use docs as a folder name.

That being said, which ever name y'all choose, we'll be able to publish it on the OTel website. So the choice is yours.

@AlexanderWert
Copy link
Member Author

AlexanderWert commented Jul 6, 2023

Originally, to avoid too much churn all at once, I wanted to delay on this aspect and just focus on refactoring the human-readable structure and website.

@jsuereth If we want to eventually rename the specification directory (to docs, or any other name) then here's another argument for doing it now, rather than later:

In several other OTel repos there are many references to the .md files (so far to the open-telemetry/opentelemetry-specification GitHub repo). Here, an example from the opentelemetry-java-instrumentation repo. We will need to change these links in all those repos to point to the semantic conventions .md files in this repo. So, it would be good if we do that change only once, as the tail of that is quite long.

@chalin
Copy link
Contributor

chalin commented Jul 6, 2023

In several other OTel repos there are many references to the .md files (so far to the open-telemetry/opentelemetry-specification GitHub repo). Here, an example from the opentelemetry-java-instrumentation repo. We will need to change these links in all those repos to point to the semantic conventions .md files in this repo. So, it would be good if we do that change only once, as the tail of that is quite long.

Even better IMHO, would be to change those links to refer to the OTel website semconv pages, once those land. That way we can gracefully handle page renames via redirects.

@AlexanderWert
Copy link
Member Author

I created an alternative PR with a renaming to docs as proposed above:

@Oberon00
Copy link
Member

Oberon00 commented Jul 6, 2023

I think we should definitely rename the yaml directory to something other than semantic_conventions. Everything else, I have no strong opinion on. We could even consider moving everything inside the current specification/ directory one level up and move most of the other files there to a new subdirectory.

(Since I just noticed it, there is also the issue of the supplementary-guidelines directory which was split out because it is not "specification". An issue that might not apply anymore if we decide to rename "specification" to anything else. I think it would be best to move these files back together, but I already suggested not splitting them in the first place in open-telemetry/opentelemetry-specification#3212 (comment). This can be done even after the release though, since I doubt there are many links to that directory)

Signed-off-by: Alexander Wert <alexander.wert@elastic.co>
@AlexanderWert
Copy link
Member Author

Originally, to avoid too much churn all at once, I wanted to delay on this aspect and just focus on refactoring the human-readable structure and website.

Want to bring another aspect here

https://github.com/search?q=repo%3Aopen-telemetry%2Fopentelemetry-java-instrumentation%20specification%2Ftrace%2Fsemantic_conventions&type=code

@jsuereth
Copy link
Contributor

jsuereth commented Jul 6, 2023

@AlexanderWert Yeah, those document references, ideally, will move to opentelemetry.io over time where we can better stabilize the URLs, as @chalin points out.

I really like #166, so I'd vote for making these changes now before release and fixing this quickly. I'll set aside more time in the AM tomorrow to kick of cleanup.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants