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 db metric name to semantic conventions #3115

Merged

Conversation

shalevr
Copy link
Member

@shalevr shalevr commented Jan 5, 2023

Description

I started to implement dbapi metrics instrumentation and
I figure out that db.client.connections.usage metric name is missing in the semantic conventions

According to the specs: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/semantic_conventions/database-metrics.md

Type of change

  • New feature (non-breaking change which adds functionality)

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:

  • No.

Checklist:

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

@shalevr shalevr requested a review from a team January 5, 2023 10:58
@srikanthccv
Copy link
Member

Thanks for the PR, but these are auto-generated files and are not to be modified by hand (although nothing prevents you from doing it). Here is the script that takes the spec version and produces the semconv files https://github.com/open-telemetry/opentelemetry-python/blob/main/scripts/semconv/generate.sh

@srikanthccv
Copy link
Member

Ah, sorry, I see this is a DB semconv which was manually added. There was some issue which prevented us from making this automated. I think we can merge this. It would be better if we could auto-generate as we do for the tracing.

@shalevr
Copy link
Member Author

shalevr commented Jan 6, 2023

Ohh I didn't know it should be auto-generated I saw that the http metric names are manually added.
What is the issue that prevented us to make it automated?
Can you merge this PR? and I will try to fix the issue and make it auto-generated
Do you know if there is an open issue with that issue?

@srikanthccv
Copy link
Member

This one #2977

@shalevr
Copy link
Member Author

shalevr commented Jan 6, 2023

Thanks!
Please add skip changelog label

@srikanthccv
Copy link
Member

I would suggest adding an entry since external users can consume this change.

@shalevr
Copy link
Member Author

shalevr commented Jan 6, 2023

Ohh I see, Thank you for the comment

@lzchen lzchen merged commit f5fb6b1 into open-telemetry:main Jan 11, 2023
keithkroeger added a commit to fidelity-contributions/open-telemetry-opentelemetry-python that referenced this pull request Jan 13, 2023
* main:
  fixed all instances of @tracer.start_as_current_span("name"): to @tracer.start_as_current_span("name") as decorators do not have colons (open-telemetry#3127)
  Add attribute name to type warning message. (open-telemetry#3124)
  Fix requirements file for example (open-telemetry#3126)
  Add db metric name to semantic conventions (open-telemetry#3115)
  Adds environment variables for log exporter (open-telemetry#3037)
  Fix bug in example (open-telemetry#3111)
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.

3 participants