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

Add end_time_mono to telemetry :stop events #1174

Merged
merged 1 commit into from
Jun 7, 2022

Conversation

cheerfulstoic
Copy link
Contributor

The Spandex library needs a 'completion_time' and it's not possible to get the start time at the beginning of the span to calculate it based on the duration

The Spandex library needs a 'completion_time' and it's not possible to get the start time at the beginning of the span to calculate it based on the duration
@benwilson512 benwilson512 merged commit 2768d10 into absinthe-graphql:master Jun 7, 2022
@benwilson512
Copy link
Contributor

Thanks!

@andrewhr
Copy link
Contributor

Hey @cheerfulstoic, giving that's not released yet, are you ok with renaming that to monotonic_time like in :telemetry.span/3?

Reason is, at #1170 I propose some changes and end up using the telemetry.span/3 as implementation detail. Assuming changes at that PR are not an option, I still argue for using the same names just for consistency - sort of unofficial convention.

What do you think?

@cheerfulstoic
Copy link
Contributor Author

So do you mean that start_time_mono and end_time_mono would both become monotonic_time?

I'm fine with it, though if start_time_mono is changing I think that would be a breaking change, and I'd love for my change to go out sooner than that 😄 But that's probably still possible

@benwilson512
Copy link
Contributor

@cheerfulstoic start_time_mono is not changing. end_time_mono is monotonic. start_time_mono is already monotonic.

@cheerfulstoic
Copy link
Contributor Author

Yeah, that's fine, but @andrewhr was suggesting changing the key to monotonic_time. Up to you, for sure 😄

@benwilson512
Copy link
Contributor

Apologies, didn't read the full thread. @andrewhr we can add keys but we can't remove any without making it backwards incompatible.

@andrewhr
Copy link
Contributor

My apologies here, some unnecessary noise due some misunderstanding of mine.

The original intend was to leverage the :telemetry conventions. Both start and end events produce a monotonic_time measurement, which is equivalent to ours start_time_mono and end_time_mono. You can see details here.

The proposal was to use those attributes names. My wrong assumption was both start_time_mono and end_time_mono weren't released yet, so reverting them would not break any production code.

Sorry again for the noise, and thanks for pointing out the breaking change!

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