Skip to content
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.

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

Strengthen versioning requirements #6970

Open
jack-berg opened this issue Dec 18, 2024 · 3 comments
Open

Strengthen versioning requirements #6970

jack-berg opened this issue Dec 18, 2024 · 3 comments
Labels
Feature Request Suggest an idea for this project help wanted

Comments

@jack-berg
Copy link
Member

We do two things that are problematic for consumers:

1. Stable artifacts can take implementation dependencies on experimental artifacts

As documented here. We don't let classes from the experimental artifacts enter the public API surface area. And by keeping it an implementation dependency, we require that consumers explicitly add their own dependency on the experimental artifact in order to use the experimental features.

This logic checks out to me, but there's still a perception problem when you've only included stable artifacts, yet see -alpha artifacts when you run ./gradlew dependencies.

Luckily, we've come a long way and there are very few experimental artifacts left. The ones that do exist are almost all "experimental by design" and will never change.

With a few changes, we can strengthen this guarantee and only allow stable artifacts to have compileOnly dependencies on experimental artifacts. compileOnly dependencies don't show up in the published *.pom file or ./gradlew dependencies of projects that depend on stable artifacts.

#6944 shows how we can accomplish this. I think we ought to take it a step further and add build tooling to verify we're following this guidance, just to prevent slip ups.

2. Various artifacts make use of shared internal code

For example, consider ConfigUtil which is used by various parts of the project outside the opentelemetry-api where the class resides, such as DebugConfig in opentelemetry-sdk-metrics.

This forces our users to make sure that versions of all artifacts are aligned, for which we recommended using a BOM. If versions aren't align, users could encounter a runtime error. For example, if ConfigUtil makes an allowed breaking change to its API and the versions of opentelemetry-api and opentelemetry-sdk-metrics are not aligned, then DebugConfig is prone to calling an API that doesn't exist.

Problem is, BOMs aren't bulletproof - there are other factors that influence the resolved dependency version, as we've seen numerous times the spring dependency management plugin.

Ideally, our artifacts should be resilient enough to work even when versions minor versions are not aligned.

I believe we can accomplish this if we ban use of shared internal code. If a particular artifact needs internal code from another, we can either:

  • Consider promoting that to publish API surface area where it gets strong compatibility guarantees
  • Make a copy of that code in the other artifact

The result would be that every artifact is complete self contained, except for calls to public API from transitive dependencies.

@jack-berg
Copy link
Member Author

#6978 adds a new architecture check to :all which finds all the instances where we use shared internal code. Current state of affairs is available here.

Some common threads (but not an exhaustive analysis):

Initialize autovalue {Signal}Data implementations

Method <io.opentelemetry.opencensusshim.MetricAdapter.convert(io.opentelemetry.sdk.resources.Resource, io.opencensus.metrics.export.Metric)> calls method <io.opentelemetry.sdk.metrics.internal.data.ImmutableMetricData.createDoubleGauge(io.opentelemetry.sdk.resources.Resource, io.opentelemetry.sdk.common.InstrumentationScopeInfo, java.lang.String, java.lang.String, java.lang.String, io.opentelemetry.sdk.metrics.data.GaugeData)> in (MetricAdapter.java:92)

Implementation / usage of experimental code

ExtendedTextMapGetter

Method <io.opentelemetry.api.baggage.propagation.W3CBaggagePropagator.extract(io.opentelemetry.context.Context, java.lang.Object, io.opentelemetry.context.propagation.TextMapGetter)> checks instanceof <io.opentelemetry.context.propagation.internal.ExtendedTextMapGetter> in (W3CBaggagePropagator.java:100)

Declarative config's StructuredConfigProperties, ComponentProvider

Method <io.opentelemetry.exporter.internal.ExporterBuilderUtil.configureExporterMemoryMode(io.opentelemetry.sdk.autoconfigure.spi.internal.StructuredConfigProperties, java.util.function.Consumer)> has parameter of type <io.opentelemetry.sdk.autoconfigure.spi.internal.StructuredConfigProperties> in (ExporterBuilderUtil.java:0)
Class <io.opentelemetry.exporter.logging.internal.ConsoleLogRecordExporterComponentProvider> implements interface <io.opentelemetry.sdk.autoconfigure.spi.internal.ComponentProvider> in (ConsoleLogRecordExporterComponentProvider.java:0)

ScopeConfigurator

Method <io.opentelemetry.sdk.logs.SdkLoggerProviderBuilder.setLoggerConfigurator(io.opentelemetry.sdk.internal.ScopeConfigurator)> calls method <io.opentelemetry.sdk.internal.ScopeConfigurator.toBuilder()> in (SdkLoggerProviderBuilder.java:125)

Compressor

Method <io.opentelemetry.exporter.sender.okhttp.internal.GrpcRequestBody.writeTo(okio.BufferedSink)> calls method <io.opentelemetry.exporter.internal.compression.Compressor.compress(java.io.OutputStream)> in (GrpcRequestBody.java:72)

Exporter common utilities

Marshaler

Method <io.opentelemetry.exporter.logging.otlp.internal.logs.OtlpStdoutLogRecordExporter.createMarshaler(io.opentelemetry.exporter.logging.otlp.internal.writer.JsonWriter, io.opentelemetry.sdk.common.export.MemoryMode, boolean)> calls method <io.opentelemetry.exporter.internal.otlp.logs.ResourceLogsMarshaler.create(java.util.Collection)> in (OtlpStdoutLogRecordExporter.java:66)
Method <io.opentelemetry.exporter.logging.otlp.internal.writer.LoggerJsonWriter.write(io.opentelemetry.exporter.internal.marshal.Marshaler)> calls method <io.opentelemetry.exporter.internal.marshal.Marshaler.writeJsonToGenerator(com.fasterxml.jackson.core.JsonGenerator)> in (LoggerJsonWriter.java:36)

ExporterMetrics

Constructor <io.opentelemetry.exporter.zipkin.ZipkinSpanExporter.<init>(io.opentelemetry.exporter.zipkin.ZipkinSpanExporterBuilder, zipkin2.reporter.BytesEncoder, zipkin2.reporter.BytesMessageSender, java.util.function.Supplier, io.opentelemetry.exporter.zipkin.OtelToZipkinSpanTransformer)> calls method <io.opentelemetry.exporter.internal.ExporterMetrics.createHttpProtobuf(java.lang.String, java.lang.String, java.util.function.Supplier)> in (ZipkinSpanExporter.java:60)

suppressInstrumentation

Method <io.opentelemetry.exporter.zipkin.ZipkinSpanExporter.export(java.util.Collection)> calls method <io.opentelemetry.api.internal.InstrumentationUtil.suppressInstrumentation(java.lang.Runnable)> in (ZipkinSpanExporter.java:80)

Internal utility classes that could be public

ApiUsageLogger

Method <io.opentelemetry.api.incubator.trace.ExtendedDefaultTracer$NoopSpanBuilder.setParent(io.opentelemetry.context.Context)> calls method <io.opentelemetry.api.internal.ApiUsageLogger.log(java.lang.String)> in (ExtendedDefaultTracer.java:62)

ThrottlingLogger

Constructor <io.opentelemetry.exporter.internal.grpc.GrpcExporter.<init>(java.lang.String, java.lang.String, io.opentelemetry.exporter.internal.grpc.GrpcSender, java.util.function.Supplier)> calls constructor <io.opentelemetry.sdk.internal.ThrottlingLogger.<init>(java.util.logging.Logger)> in (GrpcExporter.java:33)

DynamicPrimitiveLongList

Method <io.opentelemetry.exporter.internal.marshal.MarshalerUtil.sizeRepeatedUInt64(io.opentelemetry.exporter.internal.marshal.ProtoFieldInfo, io.opentelemetry.sdk.internal.DynamicPrimitiveLongList)> calls method <io.opentelemetry.sdk.internal.DynamicPrimitiveLongList.isEmpty()> in (MarshalerUtil.java:174)

checkArgument

Method <io.opentelemetry.exporter.prometheus.PrometheusHttpServerBuilder.setHost(java.lang.String)> calls method <io.opentelemetry.api.internal.Utils.checkArgument(boolean, java.lang.String)> in (PrometheusHttpServerBuilder.java:56)

StringUtils isNullOrEmpty, paddLeft

Method <io.opentelemetry.extension.trace.propagation.JaegerPropagator.isFlagsValid(java.lang.String)> calls method <io.opentelemetry.api.internal.StringUtils.isNullOrEmpty(java.lang.String)> in (JaegerPropagator.java:293)
Method <io.opentelemetry.extension.trace.propagation.OtTracePropagator.extract(io.opentelemetry.context.Context, java.lang.Object, io.opentelemetry.context.propagation.TextMapGetter)> calls method <io.opentelemetry.api.internal.StringUtils.padLeft(java.lang.String, int)> in (OtTracePropagator.java:106)

DaemonThreadFactory

Constructor <io.opentelemetry.sdk.logs.export.BatchLogRecordProcessor.<init>(io.opentelemetry.sdk.logs.export.LogRecordExporter, io.opentelemetry.api.metrics.MeterProvider, long, int, int, long)> calls constructor <io.opentelemetry.sdk.internal.DaemonThreadFactory.<init>(java.lang.String)> in (BatchLogRecordProcessor.java:83)

AttributesMap

Method <io.opentelemetry.sdk.logs.SdkLogRecordBuilder.setAttribute(io.opentelemetry.api.common.AttributeKey, java.lang.Object)> calls method <io.opentelemetry.sdk.internal.AttributesMap.create(long, int)> in (SdkLogRecordBuilder.java:105)

ComponentRegistry

Method <io.opentelemetry.sdk.logs.SdkLoggerBuilder.build()> calls method <io.opentelemetry.sdk.internal.ComponentRegistry.get(java.lang.String, java.lang.String, java.lang.String, io.opentelemetry.api.common.Attributes)> in (SdkLoggerBuilder.java:39)

WeakConcurrentMap

Method <io.opentelemetry.exporter.internal.otlp.ResourceMarshaler.create(io.opentelemetry.sdk.resources.Resource)> calls method <io.opentelemetry.context.internal.shaded.WeakConcurrentMap.get(java.lang.Object)> in (ResourceMarshaler.java:33)

@vasantteja
Copy link
Contributor

@jack-berg can I please work on this?

@jack-berg
Copy link
Member Author

@jack-berg can I please work on this?

This is a sort of a overall tracking issue which if we agree on a path forward, will likely have many contributing PRs over a long time. In today's 1/9 java SIG, we discussed this. Some notes:

  • General consensus that this is a good aspirational goal.
  • The first point is easier and attainable through PR Remove -alpha artifacts from runtime classpath of stable components #6944. Nice.
  • The second point will necessarily stretch out over a long time horizon. Add arch test to detect usage of shared internal code #6978 establishes the groundwork for tracking progress by adding code to programmatically detect shared internal code.
  • On the point of removing shared internal code, we discussed:
    • Is there advantage to apply some prioritization scheme to removal of shared internal code? I.e. what if we first targeting removal of shared internal code from opentelemetry-api?
    • Do we need to remove shared internal code amongst related groups of artifacts? I.e. is it ok for artifacts within the opentelemetry-sdk-* family to use shared internal code?
    • If we do want eliminate shared internal code altogether, we need to find a strategy to indicate that code is public and comes with backwards compatibility guarantees, but still not meant / designed to be called directly by users. One option is to introduce a new convention where we put such code in a *.util.* package and include standard javadoc boilerplate indicating intended use.

I'd say if you want to help, take a look at reviewing #6944 and #6978 for now. After than, we can work out strategies for beginning to tackle the large task of removing shared internal code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Request Suggest an idea for this project help wanted
Projects
None yet
Development

No branches or pull requests

2 participants