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

feat(*): Implement the skeleton of an OTEL observability system #2348

Merged
merged 1 commit into from
Mar 22, 2024

Conversation

calebschoepp
Copy link
Collaborator

@calebschoepp calebschoepp commented Mar 11, 2024

Summary

This PR introduces OTEL observability into Spin — specifically tracing. Using the nomenclature of #2293 I have added some trigger observability while also cleaning up a bit of the runtime observability. This PR DOES NOT provide full tracing across the entirety of the Spin codebase. That would make for a huge PR so I opted to only trace a few parts and full tracing can be added in a followup PR.

The goal of this PR is to land in a place we're happy with the DX around observability leaving room for us to improve it in future PRs.

Testing

# Install the otel-cli for easily producing distributed traces
go install github.com/equinix-labs/otel-cli@latest

# Run Jaeger
docker run -d -p16686:16686 -p4317:4317 -p4318:4318 -e COLLECTOR_OTLP_ENABLED=true jaegertracing/all-in-one:latest

# Run the app
OTEL_EXPORTER_OTLP_ENDPOINT=http://localhost:4318 spin up

# Call the app with the otel-cli
OTEL_EXPORTER_OTLP_ENDPOINT=localhost:4317 otel-cli exec -- bash -c 'curl localhost:3000-v -H "traceparent: $TRACEPARENT"'

@calebschoepp calebschoepp force-pushed the observability-start branch 2 times, most recently from 2e34a38 to 118df50 Compare March 11, 2024 23:38
crates/telemetry/src/config.rs Outdated Show resolved Hide resolved
crates/telemetry/src/config.rs Outdated Show resolved Hide resolved
crates/telemetry/src/config.rs Outdated Show resolved Hide resolved
crates/telemetry/src/traces.rs Outdated Show resolved Hide resolved
crates/telemetry/src/traces.rs Outdated Show resolved Hide resolved
crates/telemetry/src/traces.rs Outdated Show resolved Hide resolved
crates/trigger-http/src/handler.rs Show resolved Hide resolved
src/bin/spin.rs Outdated Show resolved Hide resolved
@calebschoepp
Copy link
Collaborator Author

@lann any ideas on how to test this feature?

@lann what are the steps to landing this. Any other stakeholders I should pull in for review other than you?

@lann
Copy link
Collaborator

lann commented Mar 15, 2024

@lann any ideas on how to test this feature?

It would probably be fine to do most of the e2e testing in spinkube since most users of telemetry will be there in the medium term. For Spin itself I would probably just test that things don't entirely blow up when telemetry is 1) disabled, 2) enabled without debug traces, 3) enabled with debug traces.

@lann
Copy link
Collaborator

lann commented Mar 15, 2024

@lann what are the steps to landing this. Any other stakeholders I should pull in for review other than you?

I would suggest landing this asap after basic "things don't blow up" tests are in place. We'll want to iterate on user feedback.

@calebschoepp
Copy link
Collaborator Author

@lann any ideas on how to test this feature?

It would probably be fine to do most of the e2e testing in spinkube since most users of telemetry will be there in the medium term. For Spin itself I would probably just test that things don't entirely blow up when telemetry is 1) disabled, 2) enabled without debug traces, 3) enabled with debug traces.

I'll obviously test those cases in Spin locally. But, I'm wondering if we have any sort of e2e testing mechanism in Spin that I could use to automate the verification of it not blowing up.

@rylev
Copy link
Collaborator

rylev commented Mar 18, 2024

@calebschoepp We have integration tests that you might be able to cargo cult to fit your needs.

@lann
Copy link
Collaborator

lann commented Mar 18, 2024

@calebschoepp We have integration tests that you might be able to cargo cult to fit your needs.

Yeah you could even run jaeger as a testing-framework service and smoke test the actual span delivery.

@calebschoepp
Copy link
Collaborator Author

calebschoepp commented Mar 20, 2024

@lann @rylev @itowlson this PR is in a place where I'm happy with it and it is ready for a final review.

Once this is approved I'll clean up the commit history.

Copy link
Collaborator

@rylev rylev left a comment

Choose a reason for hiding this comment

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

Looks great!

crates/telemetry/src/lib.rs Outdated Show resolved Hide resolved
crates/trigger-http/src/lib.rs Show resolved Hide resolved
tests/integration.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@itowlson itowlson left a comment

Choose a reason for hiding this comment

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

90% of my comments are nits or sylistic fretting, and are non-blockers. The one thing I'm concerned about is the error/exit behaviour in main. If I've misread that (which is entirely likely!), let me know and I'll change to an approve!

Thank you for labouring through this. It's a huge plumbing task, but super useful.

crates/telemetry/src/config.rs Outdated Show resolved Hide resolved
crates/telemetry/src/config.rs Outdated Show resolved Hide resolved
crates/telemetry/src/config.rs Outdated Show resolved Hide resolved
crates/telemetry/src/config.rs Outdated Show resolved Hide resolved
crates/telemetry/src/config.rs Outdated Show resolved Hide resolved
crates/telemetry/src/detector.rs Outdated Show resolved Hide resolved
crates/telemetry/src/detector.rs Outdated Show resolved Hide resolved
crates/trigger-http/src/lib.rs Show resolved Hide resolved
crates/trigger-http/src/lib.rs Show resolved Hide resolved
src/bin/spin.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@itowlson itowlson left a comment

Choose a reason for hiding this comment

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

Yay!

src/bin/spin.rs Outdated Show resolved Hide resolved
crates/telemetry/src/config.rs Outdated Show resolved Hide resolved
crates/telemetry/src/config.rs Outdated Show resolved Hide resolved
crates/telemetry/src/config.rs Outdated Show resolved Hide resolved
crates/telemetry/src/detector.rs Show resolved Hide resolved
crates/telemetry/src/lib.rs Outdated Show resolved Hide resolved
crates/telemetry/src/detector.rs Show resolved Hide resolved
src/bin/spin.rs Outdated Show resolved Hide resolved
@calebschoepp calebschoepp merged commit f7cb8ae into fermyon:main Mar 22, 2024
15 checks passed
@calebschoepp calebschoepp deleted the observability-start branch March 22, 2024 15:26
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.

4 participants