-
Notifications
You must be signed in to change notification settings - Fork 829
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
Cleanup TraceFlags Alternatives #2702
Comments
As commented on the PR, I think I prefer option (2) (#2698), but not by a large margin. I think my feeling is mostly about the size of the change, rather than the precise details of the change itself. Clearly, either will work. |
The smallest size will probably be 3 or 4 because they don't touch SpanContext that much (only remove a function). Keep in mind that in the second PR I have both options maybe I should split to show the size |
I think it was the requirement of having |
That is option 4. I will split it to avoid confusion. |
oh shoot. I missed that the 2 were independent. :) |
@jkwatson done, you can now see the 2 options separately :) |
I haven't gotten to do a detailed look through the PRs yet, but #2 seems reasonable to me - users would call |
After looking back and forth at the 4 options like 6 times, I think I slightly prefer #2701 (option 3), but really, I think the number of users for these changes are so miniscule that it doesn't matter all that much to me. |
@anuraaga I expect users to call isRecording on the Span not the isSampled, so for users that only instrument code is probably a noop |
@jkwatson options 3 and 4 are the most performant, options so I slightly prefer one of these, but option 2 is slightly more consistent with the IDs. If I were to vote I would go for 3,4,2 (I really don't want to ignore the cleanup so no vote for 1) |
Modification of 2, since there are only 255 possibilities, we could have 255 * (class TraceFlag { String hex; byte val; boolean sampled; }) and just have |
The problem is that it is very fast to search in that 255 list by byte number but to search by string is required to either transform the string into number at that point you just do a bit and or have a map which is slower. A 2' option where we return the class (one of the 255) is as fast as the the bytes. But if users will have a String in their hand is hard to lookup by that in the list of classes. |
So we can do a 2' where we return a class TraceFlags and pre-construct all of them. That is another possibility |
Decision was to move forward with option 5 |
There are some options proposed:
We should decide on one of the options ASAP and implement (if needed).
The text was updated successfully, but these errors were encountered: