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.

Sign up for GitHub

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

Jump to bottom

Wip fix for trace 1.0 #70

Merged

jsuereth merged 10 commits into master from wip-fix-for-trace-1.0

Jan 25, 2021

Collaborator

jsuereth commented Jan 22, 2021

Update readme and build so that failures to run the mock server are super obvious
Update all APIs to 0.14.1 pending the 1.0 release.
Overhaul MetricExporter due many changes in the SDK around "type safe point usage" and known issues in the current impl
- Make room for solving MetricExporter doesn't work with tags #64 via new "builder" algorithm design.
- Make room for solving Don't call createMetricDescriptor on every Export call #68 via new design for exporter where metric descriptors are first aggregated before calling create.
- Remove timestamp throttling pending design on Better Errors for users when Metrics are submitted in too quick succession or out of order #69

jsuereth added 6 commits

January 22, 2021 09:55


          Fix readme to ensure test failures are easily remedied.

7e47378

- Create an absurdly named exception to help gradle test failures tell users how to fix issues.
- Add better `--info` output on test failure for mock server
- Update README to show how to build the project.

Note: The best errors we get out of gradle by default look like this now:

```
> Task :exporter-metrics:test

com.google.cloud.opentelemetry.metric.EndToEndTest > testExportEmptyMetricsList FAILED
    com.google.cloud.opentelemetry.metric.MockServerStartupFailedPleaseReadmeException at EndToEndTest.java:58
        Caused by: java.io.IOException at EndToEndTest.java:42
            Caused by: java.io.IOException at EndToEndTest.java:42
    java.lang.NullPointerException at EndToEndTest.java:64
```


          improve error messages on test failures in gradle.

230229c


          Cleanups for release (maven scm incorrect) and test fixes.

a52c340


          Upgrade Trace exporter to 0.14.1 open telemetry.

c769421


          Update to metrics to 0.14.1 and refactoring.

b21191c

- Start of a refactor to improve handling of multiple-items-in-a-timeseries (see #64)
  - Add new aggregation tactic to look at ALL timeseries before generating MetricDescriptors
  - Lazy-create metric descriptors and TimeSeries as we see new metrics
  - Post-synthesis of time-series, check sanity and push to GCM.
  - Attempt to leverage new "less casting" interfaces in 0.14
- Update API/build for re-arranged 0.14.1 artifacts and metrics "alpha" stage.


          Update exmaples for 0.14.1

cbaeafa

jsuereth requested a review from aabmass

January 23, 2021 14:39

aabmass approved these changes

View reviewed changes

Contributor

aabmass left a comment

LGTM just left some nits/questions

...s/trace/src/main/java/com/google/cloud/opentelemetry/example/trace/TraceExporterExample.java Outdated Show resolved Hide resolved

exporters/metrics/src/main/java/com/google/cloud/opentelemetry/metric/MetricExporter.java Outdated

Comment on lines 126 to 127

		"Metric type {} not supported. Only gauge and cumulative types are supported.",
		metricData.getType());

Contributor

aabmass Jan 25, 2021

nit inconsistent indent

Collaborator Author

jsuereth Jan 25, 2021

fixed

exporters/metrics/src/main/java/com/google/cloud/opentelemetry/metric/MetricExporter.java Outdated

+                    // Extract all the underlying points.
+                    switch(metricData.getType()) {
+                      case LONG_GAUGE:
+                        for(LongPoint point : metricData.getLongGaugeData().getPoints()) {

Contributor

aabmass Jan 25, 2021

I'm not too familiar with the Java SDK – in practice, is there always just one point?

Collaborator Author

jsuereth Jan 25, 2021

No, that's one of the issues we're fixing here.

exporters/metrics/src/main/java/com/google/cloud/opentelemetry/metric/MetricExporter.java Outdated Show resolved Hide resolved

exporters/metrics/src/main/java/com/google/cloud/opentelemetry/metric/MetricExporter.java

+                  List<TimeSeries> series = builder.getTimeSeries();
+                  createTimeSeriesBatch(metricServiceClient, ProjectName.of(projectId), series);
+                  // TODO: better error reporting.
+                  if (series.size() < metrics.size()) {

Contributor

aabmass Jan 25, 2021

Re above comment about each MetricData having a list of points; if that's not the case, this would report failure. Should we just remove the check for now if its not adding anything?

Collaborator Author

jsuereth Jan 25, 2021

I'm trying to not change the behavior right before 1.0. I agree this needs to get fixed, but I'd rather spend some time on it.

Given metrics is alpha, I think the TODO and a bug to fix is the right way to go for now.

exporters/metrics/src/main/java/com/google/cloud/opentelemetry/metric/MetricExporter.java

+                    // }
+                  }
+                  List<TimeSeries> series = builder.getTimeSeries();
+                  createTimeSeriesBatch(metricServiceClient, ProjectName.of(projectId), series);

Contributor

aabmass Jan 25, 2021

Where do GCM API errors get surfaced? It doesn't look like createTimeSeriesBatch() throws. API errors seem like the most reasonable case to return CompletableResultCode.ofFailure()

Collaborator Author

jsuereth Jan 25, 2021

I need to refactor all of that code. I opened up an issue to track it, but yeah... it's not clear what happens right now.

Even more, we may want to use an asynchronous client to avoid blocking, as we return a completable result (i.e an async observable). We'll fix this up more post-1.0 as it won't break existing usage, just make it better.

exporters/metrics/src/test/java/com/google/cloud/opentelemetry/metric/EndToEndTest.java

Comment on lines +37 to +38

		// Start the mock server. This assumes the binary is present and in $PATH.
		// Typically, the CI will be the one that curls the binary and adds it to $PATH.

Contributor

aabmass Jan 25, 2021

This doesn't seem to assume it's in $PATH? Is the comment outdated?

Collaborator Author

jsuereth Jan 25, 2021

Yeah, I'm going to fix this up in a follow-on. I want to use test-containers here, maybe we discuss that more offline.

exporters/trace/src/test/java/com/google/cloud/opentelemetry/trace/EndToEndTest.java

Comment on lines +59 to +60

		// Start the mock server. This assumes the binary is present and in $PATH.
		// Typically, the CI will be the one that curls the binary and adds it to $PATH.

Contributor

aabmass Jan 25, 2021

Same as above

Collaborator Author

jsuereth Jan 25, 2021

same comment as above, also I'd like to share this in a test-util library component that's not released.

jsuereth and others added 4 commits

January 25, 2021 16:00


          Update examples/trace/src/main/java/com/google/cloud/opentelemetry/ex…

6f579f7

…ample/trace/TraceExporterExample.java

Co-authored-by: Aaron Abbott <[email protected]>


          Update exporters/metrics/src/main/java/com/google/cloud/opentelemetry…

ea4c3a8

…/metric/MetricExporter.java

Co-authored-by: Aaron Abbott <[email protected]>


          Style fixes.

e92c1c7


          Merge branch 'wip-fix-for-trace-1.0' of https://github.com/GoogleClou…

10299ea

…dPlatform/opentelemetry-operations-java into wip-fix-for-trace-1.0

jsuereth merged commit e2ad6d1 into master

jsuereth deleted the wip-fix-for-trace-1.0 branch

January 25, 2021 22:28

jsuereth restored the wip-fix-for-trace-1.0 branch

January 26, 2021 15:18

jsuereth deleted the wip-fix-for-trace-1.0 branch

January 26, 2021 15:20

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet