Skip to content

Better choices for stackTraceKey #1310

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

Merged
merged 1 commit into from
May 12, 2025
Merged

Conversation

rjrjr
Copy link
Contributor

@rjrjr rjrjr commented May 12, 2025

Found a few spots where stackTraceKey was including id hashes, resulting in separate crash reporter error groups per process. Fixed that and over-documented the rules. Glad this isn't public API.

@rjrjr rjrjr requested review from zach-klippenstein and a team as code owners May 12, 2025 17:18
@rjrjr rjrjr force-pushed the ray/no-id-hashes-in-group-keys branch from c572042 to ee6c2ef Compare May 12, 2025 17:27
@rjrjr
Copy link
Contributor Author

rjrjr commented May 12, 2025

  at com.squareup.workflow1.internal.RealRenderContext.fakeMethodForCrashGrouping(com.squareup.workflow1.Worker<[REDACTED]> (Kotlin reflection is not available)-key:237304290)
  • Not thrilled about Kotlin reflection is not available coming from the identifier but that's just a nit
  • That "key" is okay this time but I think it's a mistake to use it. Remembering too many UUID calls for that.

@rjrjr
Copy link
Contributor Author

rjrjr commented May 12, 2025

  at com.squareup.workflow1.internal.RealRenderContext.fakeMethodForCrashGrouping(com.squareup.workflow1.Worker<[REDACTED]> (Kotlin reflection is not available)-key:237304290)
  • Not thrilled about Kotlin reflection is not available coming from the identifier but that's just a nit
  • That "key" is okay this time but I think it's a mistake to use it. Remembering too many UUID calls for that.

Actually that kotlin reflection thing is pretty bad, the type is the only unique bit.

@rjrjr
Copy link
Contributor Author

rjrjr commented May 12, 2025

  at com.squareup.workflow1.internal.RealRenderContext.fakeMethodForCrashGrouping(com.squareup.workflow1.Worker<[REDACTED]> (Kotlin reflection is not available)-key:237304290)
  • Not thrilled about Kotlin reflection is not available coming from the identifier but that's just a nit
  • That "key" is okay this time but I think it's a mistake to use it. Remembering too many UUID calls for that.

Actually that kotlin reflection thing is pretty bad, the type is the only unique bit.

Ah, but the redacted bit had a very nice name. Okay, I think this will work. Going to look a bit harder to see if I can get the "Kotlin reflection" noise out of there, every time it shows up is irritating.

@rjrjr
Copy link
Contributor Author

rjrjr commented May 12, 2025

Ah, but the redacted bit had a very nice name. Okay, I think this will work. Going to look a bit harder to see if I can get the "Kotlin reflection" noise out of there, every time it shows up is irritating.

No wait, that's in the message, not the stack key. So the stack key should be using whatever is in the message…

@rjrjr
Copy link
Contributor Author

rjrjr commented May 12, 2025

That's the toString() of WorkflowIdentifier, which looks like it was crazy carefully over engineered for exactly this kind of case. I was avoiding it, going to go ahead and use it.

@rjrjr rjrjr force-pushed the ray/no-id-hashes-in-group-keys branch from ee6c2ef to 432800b Compare May 12, 2025 18:52
@rjrjr
Copy link
Contributor Author

rjrjr commented May 12, 2025

That's the toString() of WorkflowIdentifier, which looks like it was crazy carefully over engineered for exactly this kind of case. I was avoiding it, going to go ahead and use it.

  • I confused myself, nothing in the line I quoted is part of the message, it really was all the key. So yes, uniqueness looks good
  • that damn "Kotlin reflection is not available" bit is still in that ID to string and you know what? I'm just going to filter it the hell out right at the source.

@rjrjr
Copy link
Contributor Author

rjrjr commented May 12, 2025

That's the toString() of WorkflowIdentifier, which looks like it was crazy carefully over engineered for exactly this kind of case. I was avoiding it, going to go ahead and use it.

  • I confused myself, nothing in the line I quoted is part of the message, it really was all the key. So yes, uniqueness looks good
  • that damn "Kotlin reflection is not available" bit is still in that ID to string and you know what? I'm just going to filter it the hell out right at the source.

Ah, bliss!

  at com.squareup.workflow1.internal.RealRenderContext.fakeMethodForCrashGrouping(com.squareup.workflow1.Worker<com.squareup.squareone.core.Sq…us>:1728925657)

@rjrjr rjrjr force-pushed the ray/no-id-hashes-in-group-keys branch from 432800b to 926da97 Compare May 12, 2025 19:19
@rjrjr
Copy link
Contributor Author

rjrjr commented May 12, 2025

Bah, backing out the reflection tweak b/c of a broken unit test, WorkflowNodeTest.sideEffect_coroutine_is_named(). But I'm coming back for it!

@rjrjr rjrjr force-pushed the ray/no-id-hashes-in-group-keys branch 2 times, most recently from 6d76e25 to ae2840d Compare May 12, 2025 19:31
Found a few spots where `stackTraceKey` was including id hashes, resulting in separate crash reporter error groups per process. Fixed that and over-documented the rules. Glad this isn't public API.
@rjrjr rjrjr force-pushed the ray/no-id-hashes-in-group-keys branch from ae2840d to 22432fa Compare May 12, 2025 19:55
@rjrjr
Copy link
Contributor Author

rjrjr commented May 12, 2025

Found a better spot for .replace(" (Kotlin reflection is not available)", ""), couldn't resist putting it back in.

@rjrjr rjrjr merged commit 1389a6b into main May 12, 2025
42 checks passed
@rjrjr rjrjr deleted the ray/no-id-hashes-in-group-keys branch May 12, 2025 22:37
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