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.

Sign up for GitHub

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

Jump to bottom

Add context to metrics reporting of buffer-full events #1566

Merged
merged 12 commits into from
Jan 22, 2024
Merged

Add context to metrics reporting of buffer-full events #1566

Changes from 3 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
ee810ca
add context to metrics reporting of buffer-full events
plantfansam Jan 16, 2024
fec17cf
Update sdk/lib/opentelemetry/sdk/trace/export/batch_span_processor.rb
plantfansam Jan 17, 2024
03fb66c
Use semantic convention for code function label
plantfansam Jan 17, 2024
dfab14e
Kwargs are better; use kwargs
plantfansam Jan 17, 2024
452f96a
stray paren
plantfansam Jan 17, 2024
bc427b7
Merge branch 'main' into buffer-full-context
plantfansam Jan 19, 2024
68fe040
use __method__ to access current method
plantfansam Jan 19, 2024
6a3c754
code better
plantfansam Jan 19, 2024
6b17880
method to string
plantfansam Jan 19, 2024
40e5e89
pass spans as arg to report_dropped_spans
plantfansam Jan 19, 2024
e5166d2
cleanup
plantfansam Jan 19, 2024
ae04ce1
Merge branch 'main' into buffer-full-context
fbogsany Jan 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions sdk/lib/opentelemetry/sdk/trace/export/batch_span_processor.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ def on_finish(span)
n = spans.size + 1 - max_queue_size
if n.positive?
spans.shift(n)
report_dropped_spans(n, reason: 'buffer-full')
report_dropped_spans(n, reason => 'buffer-full', OpenTelemetry::SemanticConventions::Trace::CODE_FUNCTION => 'on_finish')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need to use hash rocket syntax to have constant as key (afaik)

end
spans << span
@condition.signal if spans.size > batch_size
Expand Down Expand Up @@ -123,7 +123,7 @@ def force_flush(timeout: nil) # rubocop:disable Metrics/MethodLength
n = spans.size + snapshot.size - max_queue_size
if n.positive?
snapshot.shift(n)
report_dropped_spans(n, reason: 'buffer-full')
report_dropped_spans(n, reason => 'buffer-full', OpenTelemetry::SemanticConventions::Trace::CODE_FUNCTION => 'force_flush')
end
spans.unshift(snapshot) unless snapshot.empty?
@condition.signal if spans.size > max_queue_size / 2
Expand Down Expand Up @@ -204,8 +204,8 @@ def report_result(result_code, batch)
end
end

def report_dropped_spans(count, reason:)
@metrics_reporter.add_to_counter('otel.bsp.dropped_spans', increment: count, labels: { 'reason' => reason })
def report_dropped_spans(count, labels = {})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Although I changed the method signature, we do not need to change other call sites for report_dropped_spans since they were using the kwarg reason: 'foo' which we can also interpret as a hash.

Copy link
Contributor

Choose a reason for hiding this comment

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

🤔 I'd rather we used named arguments consistently, and Strings for keys and values in the labels. I.e. I'd prefer:

def report_dropped_spans(count, labels: nil) ... end

report_dropped_spans(n, labels: { 'reason' => 'buffer-full', 'code.function' => 'force_flush' })

or:

def report_dropped_spans(count, reason:, function: nil)
  @metrics_reporter.add_to_counter('otel.bsp.dropped_spans', increment: count, labels: { 'reason' => reason, 'code.function' => function }.compact)
end

report_dropped_spans(n, reason: 'buffer-full', function: 'force_flush')

I prefer the latter, since it is cleaner for callers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, let's do the latter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

dfab14e

@metrics_reporter.add_to_counter('otel.bsp.dropped_spans', increment: count, labels: labels)
end

def fetch_batch
Expand Down
Loading