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.

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

SpanData interface should expose abstract time interface #6395

Closed
mleegwt opened this issue Apr 17, 2024 · 5 comments
Closed

SpanData interface should expose abstract time interface #6395

mleegwt opened this issue Apr 17, 2024 · 5 comments
Labels
Feature Request Suggest an idea for this project

Comments

@mleegwt
Copy link

mleegwt commented Apr 17, 2024

Is your feature request related to a problem? Please describe.
In the SpanData interface there are two methods that return nanos since epoch. This causes everyone to write their own conversions to something human readable.

Describe the solution you'd like
I would like and abstract object to be returned on the interface. I can imagine the methods Instant getStart() and Instant getEnd().
The simplest variant would be to add these methods and have them call (implementation) something like:

   private Instant instantFromNanosSinceEpoch(long nanosSinceEpoch) {
        long seconds = nanosSinceEpoch / 1_000_000_000; // Convert nanoseconds to seconds
        int nanos = (int) (nanosSinceEpoch % 1_000_000_000); // Get remaining nanoseconds
        return Instant.ofEpochSecond(seconds, nanos);
    }

Describe alternatives you've considered
Currently I'm converting to instant myself. Especially for testing purposes human readable is an essential part of success.

Additional context
n.a.

@mleegwt mleegwt added the Feature Request Suggest an idea for this project label Apr 17, 2024
@jack-berg
Copy link
Member

I don't think we should add new methods to the SpanData interface, since even with default implementations there's an increased opportunity for the implementation to diverge such that Instant getEnd() and long getEndEpochNanos() disagree.

Its actually simpler to convert from epoch nanos to Instant than your code suggests:

Instant.ofEpochSeconds(0L, spanData.getEndEpochNanos())

That should do the trick. Tested it myself and appears to work. Checkout this post for more info.

Given how easy it is, I'm not inclined to add any additional syntactic sugar. Closing, but feel free to re-open if I've missed something.

@mleegwt
Copy link
Author

mleegwt commented Apr 30, 2024

Thanks for the simplified code suggestion.

What do you mean with there's an increased opportunity for the implementation to diverge such that Instant getEnd() and long getEndEpochNanos() disagree?

@jack-berg
Copy link
Member

SpanData is an interface, so the SDK provides a standard implementation but its possible for users to provide their own implementation. For example, you could introduce a SpanExporter which filters or modifies the Collection<SpanData> passed to SpanExporter#export before calling a downstream exporter. This requires you to provide your own implementation of SpanData (a common pattern is to introduce a delegating SpanData which overrides select properties but delegates to the original SpanData for the rest of the accessors). Implementations of SpanData would need to be aware that there are two accessors of the end and start timestamps which need to be kept in sync with each other.

@mleegwt
Copy link
Author

mleegwt commented May 1, 2024

Clear. When providing a default implementation of getEnd() and getStart() that would not be much of an issue, right?
If performance and memory footprint was not much of an issue I would have suggested to change to API to the instant based variant. That probably won't fly for SpanData, hence this approach. Does this make you change your mind? Thanks.

@jack-berg
Copy link
Member

Even with default methods, it increases the likelihood of an incorrect implementation. Consider IDE autocomplete tools which automatically provide skeleton implementations of interfaces. Additionally, we have a repository convention to represent timestamps in epoch ns. Change it here and we ought to carry the pattern everywhere else. This, coupled with the minimal improvement in ergonomics makes me think its not worth it. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Request Suggest an idea for this project
Projects
None yet
Development

No branches or pull requests

2 participants