Update metrics sample to use OTLP and OTEL collector #606
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
davidfowl
reviewed
Dec 4, 2024
samples/Metrics/MetricsApp.AppHost/OpenTelemetryCollector/CollectorResourceBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
davidfowl
reviewed
Dec 4, 2024
samples/Metrics/MetricsApp.AppHost/OpenTelemetryCollector/CollectorResourceBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
davidfowl
reviewed
Dec 4, 2024
samples/Metrics/MetricsApp.AppHost/OpenTelemetryCollector/CollectorResourceBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
samples/Metrics/MetricsApp.AppHost/OpenTelemetryCollector/CollectorResourceBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
JamesNK
force-pushed
the
jamesnk/otelcollector
branch
from
7cd7364 to
a7fb5ff
Compare
...MetricsApp.AppHost/OpenTelemetryCollector/OpenTelemetryCollectorResourceBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
davidfowl
reviewed
Dec 5, 2024
samples/Metrics/grafana/config/provisioning/datasources/default.yaml
Outdated
Show resolved
Hide resolved
JamesNK
commented
Dec 5, 2024
Comment on lines
-49
to
+50
| var certFileDest = Path.Combine(DEV_CERT_BIND_MOUNT_DEST_DIR, certFileName); | ||
| var certKeyFileDest = Path.Combine(DEV_CERT_BIND_MOUNT_DEST_DIR, certKeyFileName); | ||
| var certFileDest = $"{DEV_CERT_BIND_MOUNT_DEST_DIR}/{certFileName}"; | ||
| var certKeyFileDest = $"{DEV_CERT_BIND_MOUNT_DEST_DIR}/{certKeyFileName}"; |
There was a problem hiding this comment.
FYI Path.Combine would add backslash when app host is run on Windows.
e.g. /dev-cert\dev-cert.pem
The OpenTelemetry collector errored when given the bad path. Seems best to hardcode to forward slash.
davidfowl
approved these changes
Dec 6, 2024
davidfowl
reviewed
Dec 6, 2024
| return Task.CompletedTask; | ||
| } | ||
|
|
||
| foreach (var resource in appModel.GetProjectResources()) |
There was a problem hiding this comment.
Shouldn't this check all resources? In the callback it would then check if OTEL_EXPORTER_OTLP_ENDPOINT is set and replace it with this value.
There was a problem hiding this comment.
The problem is that OTEL_EXPORTER_OTLP_ENDPOINT env var is added by a callback. At the resource level we don't know exactly what env vars there are from a callback.
There was a problem hiding this comment.
You can do work in the callback to look at what has been set.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The metrics sample today configures the developer app to expose a Prometheus scrape endpoint, and configures Prometheus to periodically scrape the endpoint for new data.
This PR changes the developer app to instead send telemetry to Prometheus using OTLP.
Because OTLP can only be sent to one endpoint by .NET OTEL SDK, the PR also adds the OpenTelemetry Collector to the app host. A lifecycle hook updates all developer apps to send telemetry to the collector, which then forwards it to the Aspire dashboard and Prometheus.