Skip to content

Commit

Permalink
🐛 Fix incorrect metrics endpoint (#361)
Browse files Browse the repository at this point in the history
* ✅ add unit test reproducing issue where metric reporting endpoint would only default to trace reporting endpoint if trace reporting endpoint was supplied via env var

* 🐛 fix bug with metric reporting endpoint not falling back to reporting endpoint when supplied via YAML

* ✅ add test test for non default metrics reporting endpoint

* Revert "🐛 fix bug with metric reporting endpoint not falling back to reporting endpoint when supplied via YAML"

This reverts commit 2c70107.

* 🐛 fix bug more simply

* 💡 fix comments in tests
  • Loading branch information
ryandens authored Feb 22, 2022
1 parent 35a14b0 commit c9851a7
Show file tree
Hide file tree
Showing 4 changed files with 43 additions and 3 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,9 @@ private static Reporting.Builder applyReportingDefaults(Reporting.Builder builde
if (!builder.hasEndpoint()) {
builder.setEndpoint(StringValue.newBuilder().setValue(DEFAULT_REPORTING_ENDPOINT).build());
}
if (builder.getTraceReporterType().equals(TraceReporterType.UNSPECIFIED)) {
builder.setTraceReporterType(TraceReporterType.OTLP);
}
if (!builder.hasMetricEndpoint()) {
if (TraceReporterType.OTLP.equals(builder.getTraceReporterType())) {
// If trace reporter type is OTLP, use the same endpoint for metrics
Expand All @@ -149,9 +152,6 @@ private static Reporting.Builder applyReportingDefaults(Reporting.Builder builde
StringValue.newBuilder().setValue(DEFAULT_REPORTING_ENDPOINT).build());
}
}
if (builder.getTraceReporterType().equals(TraceReporterType.UNSPECIFIED)) {
builder.setTraceReporterType(TraceReporterType.OTLP);
}
if (builder
.getMetricReporterType()
.equals(MetricReporterType.METRIC_REPORTER_TYPE_UNSPECIFIED)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -205,4 +205,38 @@ public void noneTraceExporter() throws IOException {
HypertraceConfig.DEFAULT_REPORTING_ENDPOINT,
agentConfig.getReporting().getMetricEndpoint().getValue());
}

@Test
void nonDefaultReportingEndpoint() throws IOException {
// GIVEN a config file with a non-default reporting endpoint
URL resource = getClass().getClassLoader().getResource("nonDefaultReportingEndpoint.yaml");
// WHEN we load the config
AgentConfig agentConfig = HypertraceConfig.load(resource.getPath());
// VERIFY the trace reporting type is OTLP
Assertions.assertEquals(
TraceReporterType.OTLP, agentConfig.getReporting().getTraceReporterType());
String expectedReportingEndpoint = "http://example.com:4317";
// VERIFY the trace reporting and metric reporting endpoints are both the specified value
Assertions.assertEquals(
expectedReportingEndpoint, agentConfig.getReporting().getEndpoint().getValue());
Assertions.assertEquals(
expectedReportingEndpoint, agentConfig.getReporting().getMetricEndpoint().getValue());
}

@Test
void nonDefaultMetricsEndpoint() throws IOException {
// GIVEN a config file with a non-default metric reporting endpoint
URL resource = getClass().getClassLoader().getResource("nonDefaultMetricsEndpoint.yaml");
// WHEN we load the config
AgentConfig agentConfig = HypertraceConfig.load(resource.getPath());
// VERIFY the trace reporting type is OTLP
Assertions.assertEquals(
TraceReporterType.OTLP, agentConfig.getReporting().getTraceReporterType());
// VERIFY the trace reporting endpoint is still the default value
Assertions.assertEquals(
"http://localhost:4317", agentConfig.getReporting().getEndpoint().getValue());
// VERIFY the metric reporting endpoint is the specified value
Assertions.assertEquals(
"http://example.com:4317", agentConfig.getReporting().getMetricEndpoint().getValue());
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
service_name: service
reporting:
metric_endpoint: http://example.com:4317
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
service_name: service
reporting:
endpoint: http://example.com:4317

0 comments on commit c9851a7

Please sign in to comment.