fix: Ensure tags values are strings

[szokeasaurusrex]

Ensure tag values are strings before serializing an event or a transaction to an Event dictionary.

Fixes #4391

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 84.84%. Comparing base (06ef70c) to head (3fb28cb). Report is 2 commits behind head on potel-base.

:white_check_mark: All tests successful. No failed tests found.

Additional details and impacted files

@@             Coverage Diff             @@
##           potel-base    #4459   +/-   ##
===========================================
  Coverage       84.84%   84.84%           
===========================================
  Files             144      144           
  Lines           14882    14883    +1     
  Branches         2362     2364    +2     
===========================================
+ Hits            12627    12628    +1     
- Misses           1533     1534    +1     
+ Partials          722      721    -1     

Files with missing lines	Coverage Δ
sentry_sdk/opentelemetry/span_processor.py	82.24% <100.00%> (+0.10%)	:arrow_up:
sentry_sdk/scope.py	79.87% <100.00%> (ø)

... and 1 file with indirect coverage changes

[szokeasaurusrex]

Looks like tests are failing because the non-string values were being asserted in the test suites

[szokeasaurusrex]

We discussed offline, but here's the summary:

1. Yeah, I believe we need to stringify in all of these places, as the conversion to Event is different for tags set on scope, tags set on spans, and tags set on transactions. Conversion in the serializer would not be feasible, as the serializer is unaware of the event schema, and would not know which tags need to be stringified
2. Unfortunately, this would break a before_send if the user was checking some non-string value. However, since the Event type passed to before_send already declares the tags as MutableMapping[str, str], the code as we have it right now is already violating this typing, so one could argue that this is a bugfix to align ourselves with the declared types

[sentrivana]

Re: 2: Thought about this some more. The thing about expected behavior is that it doesn't really matter if it's technically incorrect -- users come to expect it and depend on it, and we need to take that into account. Especially in this case as the event is technically part of the public API in before_send.

Another reason I'm reluctant to do this in a minor version is that in general it's hard to notice errors in a before_send. Things might just silently stop working and that's really bad UX. (There's already been a handful of issues that turned out to be problems in people's before_send that were close to impossible to diagnose as such from user point of view.)

What we can do:

• Fix this on the potel-base branch so that it goes out in a major. 3.0 is hopefully just a few weeks away so people wouldn't have to wait for the fix long.
• Find a way to push the serialization after before_send so that the tags are still accessible as-is in the callback, and only serialized later. Probably hacky.
• Introduce a deprecation period before we make the actual change. Emit a deprecation warning if we detect a non-string tag is set. This is not great because the SDK itself is setting non-string tags and those deprecation warnings would not be actionable.

Doing this on potel-base is our best bet here. The issue has been there for years and as I understand it we've only had someone encounter it recently, so fixing this in 3.0 should be sufficient.

[szokeasaurusrex]

@sentrivana I guess then there's also the question of whether we should fix this at all. The use case is pretty edge-case; if your premise is correct that folks are relying on the "incorrectly typed" tag values, then making the change in the major would also be disruptive, right?

What do you think?

[sentrivana]

Some disruption is to be expected when upgrading to a new major version. As long as we document this properly (changelog, migration guide in repo, migration guide in docs) we're good.

[szokeasaurusrex]

@sentrivana I rebased on potel-base

[reuvenstr]

Do you know when this fix will be available? I checked a release pages and didn't see it there

[sentrivana]

Hey @reuvenstr, the fix was merged into a new major branch which we've since abandoned, so this is currently not part of any release.

I've created a new issue to track the fix on an active branch: https://github.com/getsentry/sentry-python/issues/5180

Where are you encountering non-string tags? There might be a workaround in the meantime.

[reuvenstr]

Hey @reuvenstr, the fix was merged into a new major branch which we've since abandoned, so this is currently not part of any release.

I've created a new issue to track the fix on an active branch: #5180

Where are you encountering non-string tags? There might be a workaround in the meantime.

Hello, thanks for fast reply I encountered this issue with sentry-sdk==2.12.0 a long time ago and forgot about it. If I remember correctly, the envelopes created by that version of the Python package failed to upload to the Sentry when using the Sentry CLI.

[sentrivana]

If you're still encountering this, you could go over the tags in a before_send hook and make sure they're properly stringified. It's 100% on us setting non-string tags in the first place, but realistically it might take some time since to fix since it might need to go out in a major, which will probably happen early next year at the earliest.

[reuvenstr]

If you're still encountering this, you could go over the tags in a before_send hook and make sure they're properly stringified. It's 100% on us setting non-string tags in the first place, but realistically it might take some time since to fix since it might need to go out in a major, which will probably happen early next year at the earliest.

Okay, thank you