Skip to content

fix timestamp issue caused by creating the metric context too early #46

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

Merged
merged 1 commit into from
Jul 14, 2020

Conversation

jaredcnance
Copy link
Member

@jaredcnance jaredcnance commented Jul 13, 2020

Closes #45

@jaredcnance jaredcnance merged commit 0ccaad3 into master Jul 14, 2020
@jaredcnance jaredcnance deleted the jared/fix-#45 branch July 14, 2020 00:55
@hussam789
Copy link

PR Code Suggestions ✨

CategorySuggestion                                                                                                                                    Impact
Possible issue
Fix timestamp race condition

The test is comparing timestamps with a potential race condition. Since
time.time() is called before my_handler(), there's a small chance the seconds
could increment between these calls, causing the test to fail intermittently.

tests/metric_scope/test_metric_scope.py [163-165]

 # act
+logger = my_handler()
 expected_timestamp_second = int(round(time.time()))
-logger = my_handler()
  • Apply this suggestion
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies a race condition by switching the order of operations, making the test more reliable. Although it's a moderate improvement, the change is well aligned with the test's context.

Medium
  • More

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.

Creating the MetricsLogger and MetricsContext in the wrapper funtion
2 participants