Skip to content

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

Status: Reviewed PRs that need fixes

Development

Successfully merging this pull request may close these issues.

2 participants