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

feat(pymongo): Set MongoDB tags directly on span data #3290

Merged
merged 8 commits into from
Jul 17, 2024

Conversation

0Calories
Copy link
Member

Another PR to iron out some inconsistencies between the Python and Node SDKs. OTel attaches MongoDB tags directly on the span data, rather than in the tags databag. This change is necessary so that Relay can parse MongoDB spans in Python for metrics extraction.

Node SDK Spans:

Relevant MongoDB data is set directly as span data
image

Python SDK Spans:

Relevant MongoDB data is set in the tags databag
image

@0Calories 0Calories self-assigned this Jul 12, 2024
Copy link

codecov bot commented Jul 12, 2024

Codecov Report

Attention: Patch coverage is 66.66667% with 1 line in your changes missing coverage. Please review.

Project coverage is 75.98%. Comparing base (301c4b8) to head (9a2c4a3).
Report is 12 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3290      +/-   ##
==========================================
- Coverage   79.40%   75.98%   -3.42%     
==========================================
  Files         132      133       +1     
  Lines       14281    15218     +937     
  Branches     2999     3291     +292     
==========================================
+ Hits        11340    11564     +224     
- Misses       2094     2798     +704     
- Partials      847      856       +9     
Files Coverage Δ
sentry_sdk/integrations/pymongo.py 60.34% <66.66%> (-23.69%) ⬇️

... and 9 files with indirect coverage changes

Copy link
Member

@szokeasaurusrex szokeasaurusrex left a comment

Choose a reason for hiding this comment

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

@0Calories couldn't this change be breaking in some way? What if a user has filters/dashboards/etc in Sentry set up based on the tags – these would break with your proposed changes.

@0Calories
Copy link
Member Author

@szokeasaurusrex That's a good point, but consistency is still more important so that we can extract metrics for multiple SDKs. I believe the Python SDK in the near future will be implementing OTel's SDK under the hood, just like the JS SDK, so this exact change will occur and break dashboards for users who rely on these tags anyways

@szokeasaurusrex
Copy link
Member

@0Calories I think this change will still require a major version bump if it is breaking. I understand that consistency matters, but semantic versioning dictates a major version bump for any breaking change.

We are in progress with implementing POTel in Python and intend to release a major when we do so, so I guess we can include this change then.

@szokeasaurusrex
Copy link
Member

I suppose the change could be made backwards-compatible by setting this data both as tags and as data on the span. The data would of course be redundant, and we would probably need to consider implications in terms of storage costs on the server, but then we could include this change in a minor without violating semantic versioning

@0Calories
Copy link
Member Author

That makes sense to me, I'll update this to include the data on both tags and data

Copy link
Member

@szokeasaurusrex szokeasaurusrex 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 making the changes! Just a few more small things, and then I think the PR will be ready for merging

tests/integrations/pymongo/test_pymongo.py Outdated Show resolved Hide resolved
tests/integrations/pymongo/test_pymongo.py Outdated Show resolved Hide resolved
sentry_sdk/integrations/pymongo.py Show resolved Hide resolved
@0Calories
Copy link
Member Author

Thanks for the review and suggestions @szokeasaurusrex, updated the PR with the assertions and your comment

Copy link
Member

@szokeasaurusrex szokeasaurusrex left a comment

Choose a reason for hiding this comment

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

Looks good!

@0Calories
Copy link
Member Author

Awesome thanks @szokeasaurusrex! Could you also please merge the PR for me? It seems I don't have permission to do so on my own

@szokeasaurusrex
Copy link
Member

@0Calories the PR needs to be updated with the base branch before you can merge

@0Calories 0Calories merged commit 57db56c into master Jul 17, 2024
123 checks passed
@0Calories 0Calories deleted the ashanand/pymongo/mongo-tags-on-span-data branch July 17, 2024 18:38
arjennienhuis pushed a commit to arjennienhuis/sentry-python that referenced this pull request Sep 30, 2024
* feat(pymongo): Set MongoDB tags directly on span data

Co-authored-by: Daniel Szoke <[email protected]>

---------

Co-authored-by: Daniel Szoke <[email protected]>
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