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

Add Caution Note re Experimental SemConvs #3995

Merged
merged 9 commits into from
Feb 16, 2024

Conversation

mattmccleary
Copy link
Contributor

@mattmccleary mattmccleary commented Feb 13, 2024

@mattmccleary mattmccleary requested a review from a team February 13, 2024 23:14
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.

@mattmccleary - FYI, I've added an alert closing tag so that this change will at least build.

@svrnm
Copy link
Member

svrnm commented Feb 14, 2024

thanks @mattmccleary, this is an important call out. I think we will need to have it at a few other places as well for people to see and recognize it. But we can fix this as a follow up (we can turn your segment into a shortcode and put it at places where we think it is required)

@svrnm
Copy link
Member

svrnm commented Feb 14, 2024

/fix:format

Copy link
Contributor

You triggered fix:format action run at https://github.com/open-telemetry/opentelemetry.io/actions/runs/7898558642

@chalin
Copy link
Contributor

chalin commented Feb 14, 2024

Wrapping something up, will get back to this PR soon -- I have some extra comments beyond the markdown link reference issue.

@svrnm
Copy link
Member

svrnm commented Feb 15, 2024

Wrapping something up, will get back to this PR soon -- I have some extra comments beyond the markdown link reference issue.

Sure, then let's not merge this before you follow up!

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.

I've pushed a fix to the alert link-ref issue. The solution is to have the markdown link definition be placed inside the shortcode.

I've also moved the note to be before the table. It makes more sense to me that way.

All that being said, I'd like to suggest that we rework the note prose and/or the opening statement "The current status of the major functional components for OpenTelemetry is as follows", so that it is clear that we're talking about the same/related things.

That is, now I'm wondering what the status table is actually giving the status of. Can we make that clearer?

content/en/docs/languages/_index.md Outdated Show resolved Hide resolved
@mattmccleary
Copy link
Contributor Author

mattmccleary commented Feb 15, 2024

I've pushed a fix to the alert link-ref issue. The solution is to have the markdown link definition be placed inside the shortcode.

I've also moved the note to be before the table. It makes more sense to me that way.

All that being said, I'd like to suggest that we rework the note prose and/or the opening statement "The current status of the major functional components for OpenTelemetry is as follows", so that it is clear that we're talking about the same/related things.

That is, now I'm wondering what the status table is actually giving the status of. Can we make that clearer?

I agree the table isn't clear, though the title of the page is "Language APIs & SDKs" so I inferred it's a composite of API and SDK. Customers can also click into the languages to learn more. In the future, the table could be reworked to include semantic conventions but that would require some thought because it could quickly blow up the size. Since it's not clear to me how to improve the situation, I'm wondering if this could be a non-blocking follow-on item. I'm happy to think on it some more.

@chalin
Copy link
Contributor

chalin commented Feb 16, 2024

I'd be generally in agreement with what you propose. I might want to tweak the text a bit though -- let me sleep on it :).

@svrnm
Copy link
Member

svrnm commented Feb 16, 2024

I agree that the table with the status per language needs to be improved, but this should not be a concern for this PR. I'll raise a follow up issue since I also have a few ideas here

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.

PTAL at the reworked note.

content/en/docs/languages/_index.md Outdated Show resolved Hide resolved
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.

👍🏻

@chalin
Copy link
Contributor

chalin commented Feb 16, 2024

Merging. I'll let @svrnm followup with the issue / proposed rework he mentioned.

@chalin chalin merged commit e1dd019 into open-telemetry:main Feb 16, 2024
14 checks passed
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.

5 participants