-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
[WIP] Add E2E Integration Test For Adaptive Sampling Processor #5951
base: main
Are you sure you want to change the base?
[WIP] Add E2E Integration Test For Adaptive Sampling Processor #5951
Conversation
Signed-off-by: Mahad Zaryab <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5951 +/- ##
===========================================
- Coverage 96.78% 51.46% -45.33%
===========================================
Files 348 176 -172
Lines 16559 8872 -7687
===========================================
- Hits 16027 4566 -11461
- Misses 343 3863 +3520
- Partials 189 443 +254
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
# Copyright (c) 2024 The Jaeger Authors. | ||
# SPDX-License-Identifier: Apache-2.0 | ||
|
||
BINARY ?= all-in-one |
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.
all-in-one is a v1 style binary. I would prefer we test v2 version (or at least both, but v2 is higher priority and testing v1 at this point is wasted work since v1 will be EOLed in a year)
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.
@yurishkuro I was trying to test jaeger
binary but I can't seem to access port 14268. Do you know why that is? I left more details on the issue: #5717 (comment)
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
@@ -152,7 +152,7 @@ func (a *aggregator) HandleRootSpan(span *span_model.Span, logger *zap.Logger) { | |||
} | |||
samplerType, samplerParam := span.GetSamplerParams(logger) | |||
if samplerType == span_model.SamplerTypeUnrecognized { | |||
return | |||
samplerType = span_model.SamplerTypeProbabilistic |
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.
@yurishkuro what kind of a config do we want to add to perform this override?
// } | ||
// } | ||
// return false | ||
return true |
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.
@yurishkuro this causes the unit tests to fail and I believe its messing with the calculations as well. Any ideas on how we can get around this? If we don't hardcode this here however, the probability only gets calculated once.
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
- name: Setup Node.js version | ||
uses: ./.github/actions/setup-node.js |
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.
not sure you need this, unless the test specifically checks that the UI is able to render the metrics
Which problem is this PR solving?
Description of the changes
How was this change tested?
Checklist
jaeger
:make lint test
jaeger-ui
:yarn lint
andyarn test