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

fix(servicegraph): make virtual node tests reliable on Windows #37550

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mapno
Copy link
Contributor

@mapno mapno commented Jan 28, 2025

Description

The virtual node tests (TestVirtualNodeServerLabels and TestVirtualNodeClientLabels) were failing intermittently on Windows due to timing issues. This change:

  • Reduces MetricsFlushInterval and StoreExpirationLoop to 10ms
  • Adds a timeout-based polling mechanism to wait for metrics generation
  • Replaces unreliable sleep with proper metric availability checks
  • Improves error messaging for test failures

Link to tracking issue

Fixes #33679

Testing

Updates tests

Documentation

The virtual node tests (TestVirtualNodeServerLabels and TestVirtualNodeClientLabels)
were failing intermittently on Windows due to timing issues. This change:
- Reduces MetricsFlushInterval and StoreExpirationLoop to 10ms
- Adds a timeout-based polling mechanism to wait for metrics generation
- Replaces unreliable sleep with proper metric availability checks
- Improves error messaging for test failures

Fixes open-telemetry#33679
Copy link
Contributor

@pjanotti pjanotti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still draft but LGTM, minus the change log file. Thanks @mapno!

# Use this changelog template to create an entry for release notes.

# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
change_type: bug_fix
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change doesn't require a change log: it is not visible to users. You can remove this file from the change I will add the label to skip the check for a change log.

@pjanotti pjanotti added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Jan 29, 2025
// Wait for metrics to be generated with timeout
deadline := time.Now().Add(5 * time.Second)
var metrics []pmetric.Metrics
for time.Now().Before(deadline) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Side note: you can keep using the assert.Eventually or assert.EventuallyWithT, the main advantage being the error message in case of failure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
connector/servicegraph Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[connector/servicegraph] Unit test failures on Windows
2 participants