Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
RuboCop found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
|
@xuan-cao-swi any way to easily see the coverage info? Quick check there seem to be GHA integrations available. |
Yeah, I integrated the codecov.io |
cheempz
left a comment
cheempz
left a comment
There was a problem hiding this comment.
Overall i like the improvements on test description and coverage, but from spot-checking what tests are actually doing, there are gaps between what descriptions say are being tested and actual setup/assertions.
| - When code under test logs warnings or errors, **stub the logger** to capture and assert the log message. | ||
| - Use `assert_equal expected, actual` form consistently. Avoid mixing `_(x).must_equal y` and `assert_equal` in the same file. | ||
| - Use `assert_match` with regex for format patterns (e.g., UUIDs, hex strings). | ||
|
|
There was a problem hiding this comment.
Same question as above on whether the instruction needs to be duplicated.
| parent = make_span({ remote: true, sampled: true, sw: true }) | ||
| params = make_sample_params(parent: parent) | ||
|
|
||
| result = sampler.should_sample?(params) |
There was a problem hiding this comment.
Would these work?
| result = sampler.should_sample?(params) | |
| result = sampler.should_sample?(params) | |
| assert_equal TEST_OTEL_SAMPLING_DECISION::RECORD_ONLY, sample.instance_variable_get(:@decision) | |
| refute result.attributes['TriggeredTrace'] |
There was a problem hiding this comment.
Should be result. I can update these assertion.
| request_headers: headers | ||
| ) | ||
|
|
||
| parent = make_span({ remote: true, sampled: false }) |
There was a problem hiding this comment.
| parent = make_span({ remote: true, sampled: false }) | |
| # a parent decision is available but ignored because it is not from us | |
| # i.e. no sw tracestate | |
| parent = make_span({ remote: true, sampled: false }) |
| buckets: { | ||
| SolarWindsAPM::BucketType::DEFAULT => { capacity: 100, rate: 10 }, | ||
| SolarWindsAPM::BucketType::TRIGGER_RELAXED => { capacity: 100, rate: 10 }, | ||
| SolarWindsAPM::BucketType::TRIGGER_STRICT => { capacity: 100, rate: 10 } |
There was a problem hiding this comment.
Would this work to ensure relaxed bucket setting used?
| SolarWindsAPM::BucketType::TRIGGER_STRICT => { capacity: 100, rate: 10 } | |
| SolarWindsAPM::BucketType::TRIGGER_STRICT => { capacity: 0, rate: 0 } |
| flags: SolarWindsAPM::Flags::SAMPLE_START | SolarWindsAPM::Flags::TRIGGERED_TRACE, | ||
| buckets: { | ||
| SolarWindsAPM::BucketType::DEFAULT => { capacity: 100, rate: 10 }, | ||
| SolarWindsAPM::BucketType::TRIGGER_RELAXED => { capacity: 100, rate: 10 }, |
There was a problem hiding this comment.
| SolarWindsAPM::BucketType::TRIGGER_RELAXED => { capacity: 100, rate: 10 }, | |
| SolarWindsAPM::BucketType::TRIGGER_RELAXED => { capacity: 0, rate: 0 }, |
|
|
||
| result = sampler.should_sample?(params) | ||
| assert_equal TEST_OTEL_SAMPLING_DECISION::RECORD_AND_SAMPLE, result.instance_variable_get(:@decision) | ||
| assert_equal true, result.attributes['TriggeredTrace'] |
There was a problem hiding this comment.
| assert_equal true, result.attributes['TriggeredTrace'] | |
| assert_equal true, result.attributes['TriggeredTrace'] | |
| assert_equal 'trigger-trace:ok', result.tracestate['xtrace_options_response'] |
| assert_match(/^[0-9a-f]{16}-01$/, result.tracestate['sw']) | ||
| end | ||
|
|
||
| it 'updates existing tracestate for valid parent' do |
There was a problem hiding this comment.
Not seeing how this checks an existing tracestate is updated?
| assert_nil result | ||
| end | ||
|
|
||
| it 'logs warning from parsed settings' do |
There was a problem hiding this comment.
| it 'logs warning from parsed settings' do | |
| it 'updates with warning from parsed settings' do |
| describe 'http_span_metadata additional' do | ||
| before do | ||
| @sampler = TestSampler.new({ local_settings: {} }) | ||
| end | ||
|
|
||
| it 'uses defaults when attributes are missing' do | ||
| attrs = { 'http.request.method' => 'GET' } | ||
| result = @sampler.http_span_metadata(OpenTelemetry::Trace::SpanKind::SERVER, attrs) | ||
| assert result[:http] | ||
| assert_equal 'http', result[:scheme] | ||
| assert_equal 'localhost', result[:hostname] | ||
| assert_equal 0, result[:status] | ||
| end | ||
| end |
There was a problem hiding this comment.
This one is making me think that the http_span_metadata method shouldn't be setting defaults when attributes are missing--the transaction filtering should just run against an empty string which would not match/filter out the request, rather than making assumptions that the request was made to http://localhost
| metric1 = Minitest::Mock.new | ||
| metric1.expect(:data_points, []) | ||
|
|
||
| [metric1] | ||
| # After reject!, empty metrics remain so we need the exporter | ||
| # The patch calls super if any data_points are present | ||
| # When all are empty it should return SUCCESS |
There was a problem hiding this comment.
I don't understand the metric1 setup here, and the comment about "empty metrics remain so we need the exporter".
| tracer.in_span('test_span') do |s| | ||
| span = s | ||
| end | ||
| # Span should have finished with expected name and kind |
There was a problem hiding this comment.
Is there some way to assert that on_finishing was called in the processor?
| SolarWindsAPM::Config.class_variable_get(:@@config)[:log_args] = original | ||
| end | ||
|
|
||
| it 'handles tracing_mode assignment' do |
There was a problem hiding this comment.
Test seems to be confusing this with trigger tracing mode
| SolarWindsAPM::Config[:log_traceId] = original | ||
| end | ||
|
|
||
| it 'handles string message with error-like content' do |
There was a problem hiding this comment.
Looking at logger_formatter.rb it seems to handle not just error-like content that's still a String, but an actual Exception object. If so this test should be updated to reflect.
3 participants
Description
Test (if applicable)