Skip to content

Conversation

@rjrjr
Copy link
Collaborator

@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 a team and zach-klippenstein 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
Collaborator 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
Collaborator 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
Collaborator 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
Collaborator 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
Collaborator 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
Collaborator 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
Collaborator 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
Collaborator 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
Collaborator 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.

3 participants