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

ref(tracing): Move TRANSACTION_SOURCE_* constants to Enum #3889

Merged

Conversation

mgaligniana
Copy link
Contributor

Fixes GH-2696

@mgaligniana mgaligniana marked this pull request as draft December 22, 2024 03:28
Copy link

codecov bot commented Dec 22, 2024

Codecov Report

Attention: Patch coverage is 87.80488% with 5 lines in your changes missing coverage. Please review.

Project coverage is 79.55%. Comparing base (eeedd11) to head (89160fc).
Report is 1 commits behind head on master.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
sentry_sdk/integrations/fastapi.py 50.00% 1 Missing ⚠️
sentry_sdk/integrations/gcp.py 0.00% 1 Missing ⚠️
sentry_sdk/integrations/litestar.py 50.00% 1 Missing ⚠️
sentry_sdk/integrations/starlette.py 66.66% 1 Missing ⚠️
sentry_sdk/integrations/starlite.py 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3889      +/-   ##
==========================================
+ Coverage   79.53%   79.55%   +0.02%     
==========================================
  Files         140      140              
  Lines       15517    15521       +4     
  Branches     2630     2631       +1     
==========================================
+ Hits        12341    12348       +7     
+ Misses       2340     2338       -2     
+ Partials      836      835       -1     
Files with missing lines Coverage Δ
sentry_sdk/integrations/aiohttp.py 81.25% <ø> (ø)
sentry_sdk/integrations/arq.py 89.84% <100.00%> (ø)
sentry_sdk/integrations/asgi.py 85.61% <100.00%> (ø)
sentry_sdk/integrations/aws_lambda.py 30.91% <100.00%> (ø)
sentry_sdk/integrations/celery/__init__.py 86.55% <100.00%> (ø)
sentry_sdk/integrations/chalice.py 89.18% <100.00%> (ø)
sentry_sdk/integrations/django/__init__.py 81.48% <100.00%> (ø)
sentry_sdk/integrations/grpc/aio/server.py 86.44% <100.00%> (ø)
sentry_sdk/integrations/grpc/server.py 76.31% <100.00%> (ø)
sentry_sdk/integrations/huey.py 16.66% <ø> (ø)
... and 13 more

... and 3 files with indirect coverage changes

@mgaligniana mgaligniana force-pushed the GH-2696-transaction-source-enum branch 2 times, most recently from f4bb234 to ef97faa Compare December 22, 2024 04:51
@mgaligniana
Copy link
Contributor Author

Linter is failing because of:

image

Should I fix them in a different commit?

@mgaligniana mgaligniana marked this pull request as ready for review December 22, 2024 04:54
Copy link
Member

@antonpirker antonpirker left a comment

Choose a reason for hiding this comment

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

nice! thanks for the cleanup

@antonpirker
Copy link
Member

antonpirker commented Dec 23, 2024

Linter is failing because of:

image

Should I fix them in a different commit?

no, this is because of a new release of mypy. I will fix this.

@antonpirker antonpirker enabled auto-merge (squash) December 23, 2024 09:02
@antonpirker
Copy link
Member

This breaks a lot of tests, because the full enum ends up in the envelope that is sent to Sentry and not the .value of the enum. Can you please fix this?

Copy link
Member

@antonpirker antonpirker left a comment

Choose a reason for hiding this comment

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

Please fix the tests, by making sure the .value of the enum (the string) ends up in the envelop json payload that is sent to Sentry.

auto-merge was automatically disabled December 23, 2024 13:37

Head branch was pushed to by a user without write access

@mgaligniana mgaligniana force-pushed the GH-2696-transaction-source-enum branch 4 times, most recently from b016caf to 79f20d7 Compare December 23, 2024 13:51
@mgaligniana
Copy link
Contributor Author

Thank you @antonpirker! Now linter and tests passing! 💪

@mgaligniana mgaligniana force-pushed the GH-2696-transaction-source-enum branch from 79f20d7 to e135bbc Compare January 14, 2025 13:26
@mgaligniana mgaligniana force-pushed the GH-2696-transaction-source-enum branch from e135bbc to 6609d89 Compare January 15, 2025 12:01
@antonpirker antonpirker added the Trigger: tests using secrets PR code is safe; run CI label Feb 20, 2025
@github-actions github-actions bot removed the Trigger: tests using secrets PR code is safe; run CI label Feb 21, 2025
@antonpirker antonpirker added the Trigger: tests using secrets PR code is safe; run CI label Feb 21, 2025
@github-actions github-actions bot removed the Trigger: tests using secrets PR code is safe; run CI label Feb 21, 2025
@mgaligniana
Copy link
Contributor Author

Hi Anton! Just curious why did we have to delete the .value again. Let me know if there is something I can help

@antonpirker antonpirker added the Trigger: tests using secrets PR code is safe; run CI label Feb 21, 2025
@github-actions github-actions bot removed the Trigger: tests using secrets PR code is safe; run CI label Feb 21, 2025
@antonpirker antonpirker added the Trigger: tests using secrets PR code is safe; run CI label Feb 21, 2025
@antonpirker
Copy link
Member

I just made it so one does not have to always use the .value, so the enum is easier to use and error due to missing .value are not happening. otherwise it is a really cool change and I will merge it today

@github-actions github-actions bot removed the Trigger: tests using secrets PR code is safe; run CI label Feb 24, 2025
@antonpirker antonpirker added the Trigger: tests using secrets PR code is safe; run CI label Feb 24, 2025
@github-actions github-actions bot removed the Trigger: tests using secrets PR code is safe; run CI label Feb 24, 2025
@antonpirker antonpirker added the Trigger: tests using secrets PR code is safe; run CI label Feb 24, 2025
@antonpirker antonpirker enabled auto-merge (squash) February 24, 2025 09:25
Copy link
Member

@antonpirker antonpirker left a comment

Choose a reason for hiding this comment

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

This is now ready to be merged!

@antonpirker antonpirker merged commit 189e4a9 into getsentry:master Feb 24, 2025
143 of 145 checks passed
@mgaligniana
Copy link
Contributor Author

Thank you @antonpirker !!

Comment on lines 142 to 143
def __str__(self):
return self.value
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't see this! Thanks for the explanation!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Trigger: tests using secrets PR code is safe; run CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Transaction Source Enum
2 participants