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

Ensuring that pymysql follows semantic conventions #2915

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Joice-crypto
Copy link

Description

This PR is related to issue: #1628

Type of Change

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

How Has This Been Tested?

  • Ran the following test suite to ensure functionality:
    .tox/py3-test-instrumentation-pymysql/bin/pytest instrumentation/opentelemetry-instrumentation-pymysql
    
    

Does This PR Require a Core Repo Change?

  • Yes. - Link to PR:
  • No.

Checklist:

See contributing.md for styleguide, changelog guidelines, and more.

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

@Joice-crypto Joice-crypto requested a review from a team as a code owner October 19, 2024 02:23
@@ -1,685 +0,0 @@
# Copyright The OpenTelemetry Authors
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason why we are deleting this?

Copy link
Author

Choose a reason for hiding this comment

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

I removed this file from the commit since it wasn't part of the PR I sent by mistake

Copy link
Contributor

Choose a reason for hiding this comment

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

The diff shows you are deleting the file which exists already. You shouldn't be committing that delete.

"port": "port",
"host": "host",
"user": "user",
"db.namespace": "db",
Copy link
Contributor

Choose a reason for hiding this comment

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

These are not the correct attributes to be changing for semantic conventions. Pymysql (as well as several other db instrumentations) use dbapi under the hood for it's instrumentations. You must check whether dbapi span attributes conform to the most recent db semantic conventions. dbapi is also one of the instrumentations needed for the semantic convention migration plan, so it is not as simple as changing the attributes to new values.

Copy link
Author

@Joice-crypto Joice-crypto Oct 19, 2024

Choose a reason for hiding this comment

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

So, should I ensure that dbapi follows the conventions defined in the SpanAttributes class, and by doing so, will it also apply to PyMySQL since it uses dbapi? Also, should it follow the semantic conventions defined here as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

@Joice-crypto

This issue is a bit difficult for new contributors to pick up as it involves implementing the semantic convention migration plan. I would advise picking up a different issue if you are trying to pick up good first issues,

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.

2 participants