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

Emit map as an OpenTelemetry metric #141

Merged
merged 31 commits into from
Oct 18, 2023

Conversation

smithclay
Copy link
Contributor

@smithclay smithclay commented Sep 22, 2023

Description

This emits map data as an OpenTelemetry metric. Purpose is to (optionally) allow exporting edge data in OpenTelemetry format to third party tools (including Prometheus) or solutions.

Open questions

References

Testing

I've pushed a build of this PR to my public Dockerhub. It can be installed using the following helm command and data can be sent to a Lightstep project by injecting the environment variable below. The standard OTEL SDK env vars are used.

helm upgrade network-mapper otterize/network-mapper -n otterize-system --set debug=true --set mapper.repository=smithclay --set mapper.tag=latest --set mapper.pullPolicy=Always --install

kubectl set env deployments otterize-network-mapper -n otterize-system \
  OTTERIZE_ENABLE_OTEL_EXPORT=true \
  OTEL_EXPORTER_OTLP_METRICS_ENDPOINT='https://ingest.lightstep.com:443' \
  OTEL_EXPORTER_OTLP_ENDPOINT='https://ingest.lightstep.com:443' \
  OTEL_EXPORTER_OTLP_INSECURE=false \
  OTEL_METRIC_EXPORT_INTERVAL=1s \
  OTEL_EXPORTER_OTLP_PROTOCOL=grpc \
  OTEL_EXPORTER_OTLP_METRICS_HEADERS='lightstep-access-token=your-token'

Checklist

  • I have added documentation for new/changed functionality in this PR and in github.com/otterize/docs

@github-actions
Copy link

github-actions bot commented Sep 22, 2023

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@orishoshan
Copy link
Contributor

Thank you @smithclay! Looks overall great, will take a closer look and comment more thoroughly after the weekend :) Have a great one!

@orishoshan orishoshan self-requested a review September 23, 2023 07:09
@orishoshan orishoshan added the enhancement New feature or request label Sep 23, 2023
@smithclay smithclay force-pushed the add-otel-exporter branch 2 times, most recently from 85addad to 8a9e080 Compare September 25, 2023 17:18
@smithclay
Copy link
Contributor Author

I have read and understood the CLA and hereby agree to its terms by making this Pull Request Comment.

@smithclay smithclay force-pushed the add-otel-exporter branch 5 times, most recently from 2f1ffe6 to be9c7df Compare September 25, 2023 18:15
@djspoons
Copy link

This is awesome @smithclay !

I hear you on reusing the names in the service graph connector component – and I'm fine with that for now! – but it certainly seems like we'll want to rename them in some way (since these are not part of that component... and it doesn't seem to follow many current conventions!).

One sort of related question I had: can we do better than a "counter"? Maybe it's worth discussing this in real time, but there are some benefits if we can use an updown counter or (maybe?) a gauge – though it we might not have exactly the right data to make those happen. Anyway, maybe chalk this question up as "spoons wants to better understand how we can aggregate these data points."

src/mapper/pkg/otelexporter/otel_exporter.go Outdated Show resolved Hide resolved
src/mapper/pkg/otelexporter/otel_exporter.go Outdated Show resolved Hide resolved
src/mapper/pkg/otelexporter/otel_exporter.go Outdated Show resolved Hide resolved
src/mapper/pkg/otelexporter/otel_exporter.go Outdated Show resolved Hide resolved
src/mapper/pkg/otelexporter/otel_exporter.go Outdated Show resolved Hide resolved
@orishoshan
Copy link
Contributor

@smithclay I commented on the PR :)
Regarding the semantic conventions you suggested: I see that you picked something for the network mapper, are there more semantic conventions beyond that that you suggest we add? A bit of an OpenTelemetry n00b here :)

@smithclay
Copy link
Contributor Author

One sort of related question I had: can we do better than a "counter"? Maybe it's worth discussing this in real time, but there are some benefits if we can use an updown counter or (maybe?) a gauge – though it we might not have exactly the right data to make those happen. Anyway, maybe chalk this question up as "spoons wants to better understand how we can aggregate these data points."

Just to recap an offline conversation: for now, going with Counter since that's at least consistent with how the servicegraphprocessor does it. Also like the idea of making the attribute/metric names overridable via config (which could expand the number of tools this could work with in the future that expect different inputs).

@smithclay smithclay changed the title [draft] Emit map as an OpenTelemetry metric Emit map as an OpenTelemetry metric Oct 3, 2023
@smithclay smithclay marked this pull request as ready for review October 3, 2023 00:12
@orishoshan
Copy link
Contributor

Approved! Thank you @smithclay and @MadVikingGod :)

I'm still struggling with getting the new end-to-end tests running on a fork. I was hoping to use GitHub Container Registry to hold the Docker images that would run in the Minikube running in the end-to-end tests, but looks like the GITHUB_TOKEN created by GitHub Actions for GHCR in forks can only receive "read" permissions (even to the author's own GHCR), so this isn't gonna work - the CI won't be able to push the images to GHCR.

I'll come up with another solution soon to let the CI run the end-to-end test.

@orishoshan orishoshan enabled auto-merge (squash) October 16, 2023 20:50
auto-merge was automatically disabled October 16, 2023 20:56

Head branch was pushed to by a user without write access

@orishoshan orishoshan enabled auto-merge (squash) October 18, 2023 15:52
@orishoshan orishoshan merged commit a4e38ba into otterize:main Oct 18, 2023
19 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Oct 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants