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 semconv to 1.21.0 #3251

Merged
merged 22 commits into from
Oct 2, 2023
Merged

Conversation

lmolkova
Copy link
Contributor

@lmolkova lmolkova commented Apr 7, 2023

Description

Updates semantic conventions to v 1.21.0.

As a part of HTTP semconv stabilization effort, we're trying to get instrumentation libraries closer to the final spec and implement changes in diverse set of languages.

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

tox + updated contrib repo and fixed tests

Does This PR Require a Contrib Repo Change?

Answer the following question based on these examples of changes that would require a Contrib Repo Change:

  • The OTel specification has changed which prompted this PR to update the method interfaces of opentelemetry-api/ or opentelemetry-sdk/

  • The method interfaces of test/util have changed

  • Scripts in scripts/ that were copied over to the Contrib repo have changed

  • Configuration files that were copied over to the Contrib repo have changed (when consistency between repositories is applicable) such as in

    • pyproject.toml
    • isort.cfg
    • .flake8
  • When a new .github/CODEOWNER is added

  • Major changes to project information, such as in:

    • README.md
    • CONTRIBUTING.md
  • Yes. - Link to PR: Update semconv to 1.20.0 opentelemetry-python-contrib#1746

Checklist:

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added - N/A
  • Documentation has been updated

@lmolkova lmolkova changed the title Update semconv to 1.19.0 Update semconv to 1.20.0 Apr 11, 2023
@srikanthccv
Copy link
Member

@lmolkova Please let me know if you need any help getting the tests passed. You need to update core/contrib repo SHAs in your PR since they are dependent changes. They are here and here and thanks for the pull request.

@srikanthccv
Copy link
Member

@lmolkova, are you still interested in continuing this work?

@lmolkova
Copy link
Contributor Author

lmolkova commented Jun 1, 2023

@srikanthccv Sorry I got distracted and didn't update the PRs. I can get back to it on the next week. If there is someone else interested, I'm definitely happy to pass it over or work together on it. In any case, I will get back to it and corresponding contrib change on the next week.

@srikanthccv
Copy link
Member

Thank you. There were some PRs intended to do the same thing, but we closed them in favour of yours, so It would be great if you complete this. You also kept it backwards by providing deprecated attributes, which should make it easier to get the contrib tests passed. We don't have to update the instrumentations to follow the latest semconv.

@lmolkova lmolkova marked this pull request as ready for review June 6, 2023 20:28
@lmolkova lmolkova requested a review from a team June 6, 2023 20:28
@lmolkova
Copy link
Contributor Author

lmolkova commented Jun 6, 2023

@srikanthccv This PR is ready for review (but corresponding contrib change is still in progress)

The only failure in the pipeline is public API change

- opentelemetry-semantic-conventions/src/opentelemetry/semconv/trace/__init__.py
	EventDomainValues
	NetSockFamilyValues
	DbCosmosdbOperationTypeValues
	DbCosmosdbConnectionModeValues
	OtelStatusCodeValues
	GraphqlOperationTypeValues
	RpcConnectRpcErrorCodeValues

Please make sure that all of them are strictly necessary, if not, please consider
prefixing them with an underscore to make them private.
After that, please label this PR with "Skip Public API check".

These are new enums defined in semconvs, so if you're fine with this, would you mind adding a label? I don't have permissions

@srikanthccv srikanthccv added the Approve Public API check This label shows that the public symbols added or changed in a PR are strictly necessary label Jun 7, 2023
@lmolkova
Copy link
Contributor Author

lmolkova commented Jun 9, 2023

@python-approvers could you please take a look?

@ocelotl
Copy link
Contributor

ocelotl commented Jun 26, 2023

@python-approvers could you please take a look?

Sure, reviewing... ✌️

@ocelotl
Copy link
Contributor

ocelotl commented Jun 26, 2023

Great job ✌️ I added a couple minor fixes for lint and spellcheck. The getting tests are failing because we need a change that is in this PR already. Once the contrib PR is ready we can use it to make the getting tests pass.

I still have to look into why the tracecontext test is failing 👀

@ocelotl
Copy link
Contributor

ocelotl commented Jun 26, 2023

@lmolkova from the error message of the tracecontext tests, it seems like the same error is the cause:

Traceback (most recent call last):
  File "/home/tigre/github/ocelotl/opentelemetry-python/./tests/w3c_tracecontext_validation_server.py", line 27, in <module>
    from opentelemetry.instrumentation.requests import RequestsInstrumentor
  File "/home/tigre/github/ocelotl/opentelemetry-python/.tox/tracecontext/src/opentelemetry-instrumentation-requests/instrumentation/opentelemetry-instrumentation-requests/src/opentelemetry/instrumentation/requests/__init__.py", line 79, in <module>
    from opentelemetry.util.http import (
  File "/home/tigre/github/ocelotl/opentelemetry-python/.tox/tracecontext/src/opentelemetry-util-http/util/opentelemetry-util-http/src/opentelemetry/util/http/__init__.py", line 40, in <module>
    SpanAttributes.HTTP_FLAVOR,
AttributeError: type object 'SpanAttributes' has no attribute 'HTTP_FLAVOR'

@lmolkova
Copy link
Contributor Author

thanks @ocelotl ! I fixed the indentation for all notes.

So I should work on the open-telemetry/opentelemetry-python-contrib#1746 and get it merged first before this one can move ahead, correct?

@ocelotl
Copy link
Contributor

ocelotl commented Jun 28, 2023

thanks @ocelotl ! I fixed the indentation for all notes.

So I should work on the open-telemetry/opentelemetry-python-contrib#1746 and get it merged first before this one can move ahead, correct?

Yes, please ✌️

Copy link
Member

@srikanthccv srikanthccv left a comment

Choose a reason for hiding this comment

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

One minor comment. Thank you for working on this.

CHANGELOG.md Outdated Show resolved Hide resolved
@lzchen
Copy link
Contributor

lzchen commented Aug 16, 2023

@lmolkova @ocelotl
Hey @lmolkova apologies for not updating you on this. Python SIG has indeed decided to follow the migration plan for sem conv as defined here. We have decided that we will be still releasing instrumenations in lockstep, and we as the Python community will be choosing the most important/popular http libraries to be "starting the migration plan", which consists of adding support for the env var and adding the possibility of sending both signals. With that being said, I don't think this PR has anything to do with the migration plan (updating to 1.20.0 does not affect new semantic conventions) correct? So people coming to review this can just review this as is?

@cBiscuitSurprise
Copy link

cBiscuitSurprise commented Aug 30, 2023

I posed this in another task (open-telemetry/opentelemetry-python-contrib#1627), but thought I bring it up here:

Just brainstorming: Is it possible to add a layer in opentelemetry.semconv like:

from opentelemetry.semconv import SemConv
semconv = SemConv(version="1.20.0");
semconv.span_attributes.SERVER_ADDRESS

It doesn't completely solve the coupling, but may help break up the PRs. You could simply have one relatively small CR to make the new version available for use, then incrementally update usage in all of the sub-packages in separate PRs. This could allow skipping sub-package updates if say 1.21.0, 1.22.0, etc. come along before all the sub-packages are updated. Then follow up by deprecating unused versions as all the usage is updated.

I see you mentioned that the consensus is to keep all sub-packages in lock-step like you said there, so this proposal probably isn't acceptable? It could help loosen this coupling and make it easier to keep this library up to date. As long as resource.telemetry.sdk.version is accurate for each instrumentation package.


Edit: I'm just now realizing this library is versioned in lock-step, so nvm.

@federicobond
Copy link
Member

This is almost ready to merge, right? The only things missing are solving the merge conflicts and moving the changelog entry to the Unreleased section.

@lmolkova @ocelotl can I be of any help here?

@lmolkova lmolkova changed the title Update semconv to 1.20.0 Update semconv to 1.21.0 Sep 27, 2023
Copy link
Member

@aabmass aabmass left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!

scripts/semconv/templates/semantic_metrics.j2 Outdated Show resolved Hide resolved
@lmolkova
Copy link
Contributor Author

lmolkova commented Sep 29, 2023

@ocelotl I took the liberty to reset your review status, please take another look if you want to.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approve Public API check This label shows that the public symbols added or changed in a PR are strictly necessary
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants