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

Add span and metric instrumentation to the LS metrics SDK export #590

Merged
merged 4 commits into from
Dec 20, 2023

Conversation

jmacd
Copy link
Contributor

@jmacd jmacd commented Dec 20, 2023

Description: Adds a span and a metric about the outcome of the LS metrics SDK periodic exporter. Changes the default to enable spans about the metrics SDK export. We will consider doing this for the span exporter, but it's different in that case because of the processor. Metrics SDKs don't drop, so we keep it simple in this PR w/ only metrics support.

Link to tracking Issue: Internal: LS-56746

Testing: Manual testing. Also: go work sync because otel deps were out of date w/ respect to OTel-Arrow.

Documentation: None. Self-tracing/metricing is not documented, happens as a result of the otelcol export pipeline using the globals.

@jmacd jmacd requested a review from moh-osman3 December 20, 2023 00:44
@codecov-commenter
Copy link

codecov-commenter commented Dec 20, 2023

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (132cbd3) 89.55% compared to head (029888b) 89.58%.

❗ Current head 029888b differs from pull request most recent head a59d4d4. Consider uploading reports for the commit a59d4d4 to get more accurate results

Files Patch % Lines
...htstep/sdk/metric/exporters/otlp/otelcol/client.go 83.87% 2 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #590      +/-   ##
==========================================
+ Coverage   89.55%   89.58%   +0.02%     
==========================================
  Files          62       62              
  Lines        3629     3658      +29     
==========================================
+ Hits         3250     3277      +27     
  Misses        286      286              
- Partials       93       95       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@moh-osman3 moh-osman3 left a comment

Choose a reason for hiding this comment

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

LGTM, changelog needs to be rebased I think

@jmacd jmacd merged commit 86b1e4a into main Dec 20, 2023
1 check passed
@jmacd jmacd deleted the jmacd/observe_metrics branch December 20, 2023 23:19
jmacd added a commit that referenced this pull request Jan 4, 2024
**Description:** Follows similar work from #590 on the metrics exporter
released in 1.23. This will create a span covering the export, not
metrics (for reasons explained within).

Pipelines: the trace SDK was still using the otel-go trace exporter,
while the otelcol-based trace exporter here was still experimental.
Lightstep has been using this exporter internally since its original
release, and now considers it production ready. This switches the
pipelines to always use the new exporter.

Fixes a typo: the MaxInFlightSizeMiB setting was set too large by a
factor of 1Mi in two places.

**Testing:**

Manual. The `examples/traces` function has to be modified to block in
order to see the new span come through, since the span called during
shut down will never export. I added `select{}` to the end of that main
and checked the span came through.
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.

3 participants