-
Notifications
You must be signed in to change notification settings - Fork 373
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
[SDTEST-482] Add get_time_provider setting to avoid timecop mocking the clock_gettime #3948
Conversation
…s::Time.get_time method
…nOperation when get_time_provider is set
Is this an internal usage from CI gem? Perhaps we should avoid making it public |
@TonyCTHsu the option to set Time.now provider is public and documented for customers here: In this case, I see no reason to hide this option from public usage: there are customers that used |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3948 +/- ##
=======================================
Coverage 97.85% 97.85%
=======================================
Files 1303 1303
Lines 78139 78200 +61
Branches 3894 3895 +1
=======================================
+ Hits 76461 76521 +60
- Misses 1678 1679 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
BenchmarksBenchmark execution time: 2024-09-25 13:24:52 Comparing candidate commit b03599c in PR branch Found 1 performance improvements and 0 performance regressions! Performance is the same for 22 metrics, 2 unstable metrics. scenario:tracing - Propagation - Datadog
|
|
As agreed with @TonyCTHsu, we will refrain from documenting this option as of now because we don't currently see any use case for it outside of |
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.
I like the idea of configuring in a cooperative way over forcing a mock. My question is more about the name.
time_provider
suggests that all time in the library will be provided through this provider? Do we have a means of really enforcing this consistent use? If we use Time.now
somewhere instead of this provider, we may end up with broken expectations around behavior.
Off the top of my head, I feel like this could be either:
- Commit to using this more broadly
- OR rename this to
trace_time_provider
and more narrowly bound its use. But then it doesn't belong inCore
.
WDYT? (I'm not quite sure what to do about this.)
@delner I don't have any preference on this topic, so I think whatever works for tracing/core is fine for me |
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.
Diff LGTM, I do not have an opinion on the naming/scope.
@delner I suggest let's merge it now because it solves a problem for the users, I will leave the question of whether it belongs to Core or Tracing open for now |
What does this PR do?
Adds
get_time_provider
setting similar totime_now_provider
that provides a workaround for the situation when timecop mocks Process.clock_gettime.Motivation:
Timecop v0.9.9 allows to mock monotonic clock, which might affect tests tracing. See the example of this failure here.
How to test the change?
Unit tests are provided