-
-
Notifications
You must be signed in to change notification settings - Fork 427
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
Hubs/Scopes Merge 28 - Fix breadcrumb ordering #3355
Conversation
…pes-merge-2-add-scopes
…ainScopes to rootScopes
|
@@ -663,8 +667,11 @@ public void setUnknown(@Nullable Map<String, Object> unknown) { | |||
@Override | |||
@SuppressWarnings("JavaUtilDate") | |||
public int compareTo(@NotNull Breadcrumb o) { | |||
// TODO also use nano time if equal | |||
return timestamp.compareTo(o.timestamp); | |||
int timestampCompare = timestamp.compareTo(o.timestamp); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In order to have consistent ordering, should we always use nanos?
We would also have to change the constructor to take a nano time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah makes sense, e.g. for OTEL where we likely have to create breadcrumbs after the fact. Will change it to only use nanos
in a follow up PR.
#skip-changelog
📜 Description
Use
System.nanoTime()
to have a more detailed timestamp for ordering breadcrumbs.The actual value shouldn't matter too much as breadcrumbs should be created / deserialized in order and
nanos
are only used iftimestamp
is the same.💡 Motivation and Context
To have better ordering of breadcrumbs and avoid random order when timestamp is equal. This also caused flaky tests.
💚 How did you test it?
📝 Checklist
sendDefaultPII
is enabled.🔮 Next steps