Rename ApacheHttpClient5Telemetry to ApacheHttpClientTelemetry (since already under v5 package) #12854
Conversation
github-actions
bot
added
the
test native
trask
force-pushed
the
rename-apache-http-client-5
branch
from
21004dc to
48d5fac
Compare
trask
force-pushed
the
rename-apache-http-client-5
branch
from
48d5fac to
9c3a4d0
Compare
...io/opentelemetry/instrumentation/apachehttpclient/v5_2/ApacheHttpClientTelemetryBuilder.java
Outdated
Show resolved
Hide resolved
.../main/java/io/opentelemetry/instrumentation/apachehttpclient/v5_2/internal/Experimental.java
Outdated
Show resolved
Hide resolved
| * APIs (or a version of them) may be promoted to the public stable API in the future, but no | ||
| * guarantees are made. | ||
| */ | ||
| public class Experimental { |
There was a problem hiding this comment.
Sorry, I don't fully understand why we need to create a Experimental class here. If it's an experimental class, I guest the name may be changed at least in the future. If we recommend users to use it instead of original method with @deprecated. Users may need to make a change when we change the Experimental class. Is there any discussion context about this point before?
There was a problem hiding this comment.
our stability guarantee doesn't apply to things under .internal. packages
The public API is any public class or method that is not in a package which includes the word
internal.
I may not have understood your question though, let me know if this answers or if it was something else, thanks
There was a problem hiding this comment.
The public API is any public class or method that is not in a package which includes the word
internal.
In fact, I want to know why we use an Experimental class to wrap the previous setEmitExperimentalHttpClientMetrics method.
Thank for your hints, because we need to stabilize the HTTP library here, for some experimental methods, in order to modify them easily, we put them in internal package firstly. Is it correct?
There was a problem hiding this comment.
yep, the experimental methods won't live forever, so we don't want to make them part of public API
.../main/java/io/opentelemetry/instrumentation/apachehttpclient/v5_2/internal/Experimental.java
Show resolved
Hide resolved
| public final class Experimental { | ||
|
|
||
| @Nullable | ||
| private static volatile BiConsumer<ApacheHttpClientTelemetryBuilder, Boolean> |
There was a problem hiding this comment.
Elsewhere we didn't use volatile. I guess it won't do harm but shouldn't also be needed for correctness as this field is set from the static initializer of ApacheHttpClientTelemetryBuilder.
There was a problem hiding this comment.
shouldn't also be needed for correctness as this field is set from the static initializer of
ApacheHttpClientTelemetryBuilder
oh, even if the static field is in a different class than the static initializer?
There was a problem hiding this comment.
I don't think that the barriers care where the fields are, so that at the end of ApacheHttpClientTelemetryBuilder class initializer everything that happened in any class should be visible. Also the setEmitExperimentalTelemetry needs an ApacheHttpClientTelemetryBuilder which requires that the class initializer is run.
| } | ||
| } | ||
|
|
||
| public static void setSetEmitExperimentalTelemetry( |
There was a problem hiding this comment.
maybe consider renaming to something like setEmitExperimentalTelemetryCallback or something similar to get rid of the double set in the name that looks like a typo
ApacheHttpClient5*classes have been deprecated and renamed toApacheHttpClient*Part of #12846
See #12867 for change log migration notes entry