Read environment variables for AKS metrics otlp support #4458
Conversation
trask
force-pushed
the
harskaur/aksMetrics
branch
4 times, most recently
from
4bb5788 to
732a57f
Compare
trask
force-pushed
the
harskaur/aksMetrics
branch
from
732a57f to
0f009a6
Compare
trask
changed the base branch from
main
to
trask/update-to-azure-sdk-latest
| if (useOtlpViaEnvVars) { | ||
| envVars.put("OTEL_METRICS_EXPORTER", "otlp,azure_monitor"); | ||
| envVars.put("OTEL_EXPORTER_OTLP_METRICS_ENDPOINT", FAKE_OTLP_INGESTION_ENDPOINT); | ||
| envVars.put("OTEL_EXPORTER_OTLP_PROTOCOL", "http/protobuf"); | ||
| } |
There was a problem hiding this comment.
this seems duplicative, I think we can simplify and just change useOtlpEndpoint to use the env vars (there's no specific need for us to test it also with system properties)
| private final List<String> jvmArgs = new ArrayList<>(); | ||
| private boolean useDefaultHttpPort; | ||
| private boolean useOtlpEndpoint; | ||
| private boolean useOtlpViaEnvVars; |
There was a problem hiding this comment.
then we can revert changes to this file
trask
force-pushed
the
harskaur/aksMetrics
branch
from
1e05ccc to
9aec7b3
Compare
| MetricData metricData = metricDataCollection.iterator().next(); | ||
| MetricDataMapper.updateMetricPointBuilder( | ||
| builder, metricData, metricData.getData().getPoints().iterator().next(), true, true); | ||
| builder, metricData, metricData.getData().getPoints().iterator().next(), true, true, null); |
There was a problem hiding this comment.
better to pass false explicitly (and ideally change the function from Boolean to boolean)
| builder, metricData, metricData.getData().getPoints().iterator().next(), true, true, null); | |
| builder, metricData, metricData.getData().getPoints().iterator().next(), true, true, false); |
There was a problem hiding this comment.
I set it to Boolean in case anyone would want to skip the _MS.SentToAMW flag. It would have different values.
There was a problem hiding this comment.
I think it would be good to clarify in the spec if we can skip sending this flag. That would be better whenever false to save ingestion costs. And then we wouldn't need tri-state here.
| assertThat(properties3).containsEntry("_MS.SentToAMW", "True"); | ||
| } | ||
|
|
||
| private void validateOtlpMetricsReceived(String name) throws Exception { |
There was a problem hiding this comment.
it doesn't look like name is used in this function
There was a problem hiding this comment.
can you confirm if it's correct to remove it, or if some validation on name should be added instead?
This PR will read the following env vars so that the MetricDataMapper can append the necessary attribute to metric / send the metrics to the right places:
OTEL_METRICS_EXPORTER
OTEL_METRICS_EXPORTER_OTLP_ENDPOINT
APPLICATIONINSIGHTS_METRICS_TO_LOGANALYTICS_ENABLED
Note: still in draft mode because this depends on the azure-sdk-for-java pr, and won't build until I build on top of this pr. I also need to test the e2e behavior once I have a built java agent.