Expand test coverage of OpenCensus-Shim by testing metrics integrations. #3835
Conversation
- Move metric conversion into its own utility (outside exporter)
- Add a series of tests for all open-census types.
- Update conversion code to work for ALL features of OTel metrics and
OpenCensus metrics. Notes:
- OC exemplars have wonky trace-attachments. Using crazy "no binary"
workaround.
- OC has gauge histogram. Encoding as DELTA with same start/stop
timestamp as closest OTEL equivalent
- OC has summaries, so we'll need to keep that data type around for
compatibility, unless we can tackle the raw measurements and push
them into Histograms.
…at's more amenable to autoconfiguring.
jsuereth
requested review from
anuraaga,
bogdandrutu,
carlosalberto,
jack-berg and
jkwatson
as code owners
Codecov Report
@@ Coverage Diff @@
## main #3835 +/- ##
============================================
+ Coverage 89.19% 89.38% +0.18%
- Complexity 4040 4085 +45
============================================
Files 486 491 +5
Lines 12524 12650 +126
Branches 1221 1232 +11
============================================
+ Hits 11171 11307 +136
+ Misses 933 925 -8
+ Partials 420 418 -2
Continue to review full report at Codecov.
|
opencensus-shim/src/main/java/io/opentelemetry/opencensusshim/metrics/MetricAdapter.java
Outdated
Show resolved
Hide resolved
| * @param censusMetric The OpenCensus metric to convert. | ||
| */ | ||
| public static MetricData convert(Resource otelResource, Metric censusMetric) { | ||
| // Note: we can't just adapt interfaces, we need to do full copy because OTel data API uses |
There was a problem hiding this comment.
We changed it to an interface, is there something that still needs to be switched?
https://github.com/open-telemetry/opentelemetry-java/pull/3783/files
There was a problem hiding this comment.
It's an unimplementable interface because all of the methods return AutoValues that you can't implement.
opencensus-shim/src/main/java/io/opentelemetry/opencensusshim/metrics/MetricAdapter.java
Outdated
Show resolved
Hide resolved
| String traceId = null; | ||
| if (exemplar.getAttachments().containsKey("SpanContext")) { | ||
| // We need to use `io.opencensus.contrib.exemplar.util.AttachmentValueSpanContext` | ||
| // The `toString` will be the following: |
There was a problem hiding this comment.
As an FYI - I created a test directly against the exemplar-utils library, but still did not take a dependency on it in the shim.
opencensus-shim/src/main/java/io/opentelemetry/opencensusshim/metrics/MetricAdapter.java
Outdated
Show resolved
Hide resolved
| import java.util.Collection; | ||
| import java.util.List; | ||
|
|
||
| /** Class that wraps multiple metric producers into one. */ |
There was a problem hiding this comment.
We have a similar class for exporter, should this be in the SDK as an official class? Or only has value for metrics shim since otherwise only the OTel SDK produces metrics?
There was a problem hiding this comment.
Which class are you talking about? I think I use the composite pattern in a few places, but not this one just yet.
Regarding longer term plans, post SDK stable launch, I hope to expose (via specification) a hook on SdkMeterProvider to register something like this for adapting external metric systems. For now, I just wanted better code coverage and there was a quick win for a nicer hook with the new metrics SDK. (Note: Whatever we do here + in Go will drive the OpenCensus compatiblity specification
...nsus-shim/src/test/java/io/opentelemetry/opencensusshim/OpenTelemetryMetricExporterTest.java
Outdated
Show resolved
Hide resolved
...shim/src/test/java/io/opentelemetry/opencensusshim/metrics/OpenCensusMetricProducerTest.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| tasks.named<Test>("test") { | ||
| // We must force a fork per-test class because OpenCensus pollutes globals with no restorative |
There was a problem hiding this comment.
Is it possible to split test sets to work around this to not fork on every test case? forkEvery(1) goes sloooow
There was a problem hiding this comment.
Only for some of the tests. I've tried to limit what's there to just the ones we need, but for some of the coverage I do needs to actually set up OpenCensus.
OpenCensus itself tended to use heavy mocks/mocking for various metric things. Really appreciate in OTel the global-cleaning utiities.
There was a problem hiding this comment.
Yeah if we can identify those tests that need it and split them into test suites at least it doesn't have to fork for every single one.
I think this is something that should go through the spec or something official, yes? Something like "how to enable the OpenCensus integration" so it's at least similar across languages? |
Yes, that's the plan. However, I'd like to get an implementation ahead of the spec to justify what will work across features. Also looking to make similar changes on the Go side over time (the current prototypes the OpenCensus spec is based on). |
MetricReaderFactoryas a better (available) option.Things I did not do, but wish were easier
Resourceoff the SDK in some fashion. We could pass it down theMetricReaderFactoryinterface, but looking for opinions there.