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.

Sign up for GitHub

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

Jump to bottom

Unset tags #2218

Closed
cssetian opened this issue Aug 28, 2019 · 7 comments
Closed

Unset tags #2218

cssetian opened this issue Aug 28, 2019 · 7 comments
Labels
Type: Improvement

Comments

@cssetian
Copy link

Tags can be set using scope.setTag and scope.setTags but there is only the ability to remove all tags with scope.clear. With SPA's where context changes greatly, it would be beneficial to be able to remove tags by tag name from the given scope
@jeremywiebe
Copy link

Chiming in. As I read the code, you can't unset tags by passing undefined or an empty object to setTags() either because the Hub always spreads the existing tags into the new set of tags.

See: https://github.com/getsentry/sentry-javascript/blob/master/packages/hub/src/scope.ts#L126

So a clearTags() method or the ability to send some "sentinal" value to setTags() to clear them would be extremely useful.

As it is, I'm stuck in how to clear the current set of tags from the scope.

@jtomaszewski
Copy link

Do you know what happens if you do .setTags({ tagToBeRemoved: undefined })? (Even though TS typedef definition doesn't let undefined in there.)

Is that tag being sent to Sentry and shown in there, even though it has undefined value? Or is it skipped in the Sentry UI completely?

@kamilogorek
Copy link
Contributor

@jtomaszewski undefined is not serializable, so it'll be dropped before it's even sent by JSON.stringify. null is serializable, will be sent through and will be effectively ignored on the server-side, thus skipped in the UI.

@jtomaszewski
Copy link

Thanks for the answer. So it means I can unset tags by passing undefined as their value?

Could we update the TS definition for setTag and setTags then, so that it accepts string | undefined as the tag value, not just string ?

@edzis
Copy link

edzis commented Dec 1, 2020 •
edited
Loading

In TypeScript I am using Sentry.setTag('mytagname', undefined!) and it is working. But that feels quite wrong. I recognize that for most uscases it is safer for setTag to accept just a string value, but I would like to have an official position from Sentry to help justify this wrong workaround in communication with colleagues.
Update: For undefined! to work with our rather typical eslint setup we have to use // eslint-disable-next-line @typescript-eslint/no-non-null-assertion which adds to the feeling of this being wrong.

@lobsterkatie
Copy link
Member

@edzis You're right, we should fix the types. It doesn't solve the OP's problem entirely, but at least codifies that tags can be unset.

I'm about to put up a PR. Will need to get some feedback from the rest of the team, but it seems a reasonable idea to me.

@lobsterkatie lobsterkatie mentioned this issue Dec 8, 2020
Merged
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Feb 22, 2021
@chrisbobbe
deps: Upgrade @sentry/react-native to 2.1.1, the latest.
e125056 
One thing we'd like to have is the merged PR
getsentry/sentry-javascript#3108, a type adjustment that lets us use
`undefined` as a value in `setTag` and `setTags`. The author of that
PR says that passing `undefined` for a tag's value is an unofficial
way to unset a tag from the scope. If that works, we'd like to do it
until getsentry/sentry-javascript#2218 is solved more officially.

I see just one declared breaking change at 2.0.0 [1]:

"""
- build(android): Changes android package name from
  `io.sentry.RNSentryPackage` to `io.sentry.react.RNSentryPackage`
  (Breaking). zulip#1131
"""

But autolinking handles the name change just fine, so there's
nothing special we have to do about that.

We don't have a complete libdef yet, but we seem to have good
coverage for our usage in what we've translated from the TypeScript
manually so far. So, double-check all of that against the new
version and fix a few things that were slightly wrong or out-of-date
(including by taking the relevant changes from
getsentry/sentry-javascript#3108).

[1] https://github.com/getsentry/sentry-react-native/blob/master/CHANGELOG.md#200
@kamilogorek
Copy link
Contributor

Closing the issue, as it seems like the original issue has been resolved.
Please do not hesitate to ping me if it is still relevant, and I will happily reopen and work on it.
Cheers!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Type: Improvement
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@jeremywiebe @edzis @cssetian @kamilogorek @jtomaszewski @lobsterkatie