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

Update user_agent.* wording to support multiple apps #680

Merged
merged 12 commits into from
Feb 9, 2024

Conversation

trisch-me
Copy link
Contributor

@trisch-me trisch-me commented Feb 1, 2024

Fixes #662

Changes

Merge requirement checklist

Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

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

such as microservices with multiple applications

I didn't really follow, what would be an example this? thx

@trisch-me
Copy link
Contributor Author

hey @trask sorry that it's not clear, probably I need to rephrase it anyway in this case.

I refer to the comments like we should expand user-agent to support all possible products inside user-agent and My ideal solution would be we define use agent name version to be "the outer most" or "most important" name/version found in the string. For YourApp/1.0.0 grpc-java-okhttp/1.27.2 I think this means YourApp and 1.0.0. from previous PR

@trisch-me
Copy link
Contributor Author

@trask @joaopgrassi I have rephrased it. Please check it out

@joaopgrassi
Copy link
Member

Hi @trisch-me !

We changed how the CHANGELOG.md is managed. Please take a look at https://github.com/open-telemetry/semantic-conventions/blob/main/CONTRIBUTING.md#adding-a-changelog-entry to see what needs to be done. Sorry for the disruption.

@trisch-me
Copy link
Contributor Author

hey @joaopgrassi I haven't updated the changelog because it's only a new wording without bigger changes. Please let me know if you think I need to create a new entry for this PR

@arminru arminru changed the title update wording to support multiple apps Update user_agent.* wording to support multiple apps Feb 6, 2024
@trisch-me
Copy link
Contributor Author

@joaopgrassi The same problem as in your recent PR (with WIP label), I don't have changes for changelog for this trivial PR but the job is failing and I don't have access to the labels in my current role.

@arminru
Copy link
Member

arminru commented Feb 6, 2024

@joaopgrassi The same problem as in your recent PR (with WIP label), I don't have changes for changelog for this trivial PR but the job is failing and I don't have access to the labels in my current role.

@trisch-me You can prefix the PR title with [chore] for the same effect but a changelog entry for the refinement and added note would be fine too I think.

@trisch-me
Copy link
Contributor Author

@arminru I will add changelog entry but I think we need to update our rules as well. There are editorial PRs etc, we could add labels automatically using github actions

@arminru
Copy link
Member

arminru commented Feb 6, 2024

@arminru I will add changelog entry but I think we need to update our rules as well. There are editorial PRs etc, we could add labels automatically using github actions

For editorial PRs, if either the PR title is prefixed with [chore] or the Skip Changelog label is added, the check will pass.

Since merging is always done by a person and not a workflow, that person can just add the label as the very last action right before merging the PR (or the PR author themselves adds the PR title prefix). Until merging it's not an issue if that one merge check fails except for the visual cue it carries.

@joaopgrassi joaopgrassi merged commit 44c830d into open-telemetry:main Feb 9, 2024
10 checks passed
ChrsMark pushed a commit to ChrsMark/semantic-conventions that referenced this pull request Feb 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Expand user agent to support more applications, not only browser
6 participants