gcp-observability: remove logging channel/server providers #9424
Conversation
| new InternalLoggingChannelInterceptor.FactoryImpl(logHelper, logFilterHelper), | ||
| new InternalLoggingServerInterceptor.FactoryImpl(logHelper, logFilterHelper)); | ||
| instance = new GcpObservability(sink, config); | ||
| instance.setProducer(channelInterceptorFactory, serverInterceptorFactory); |
There was a problem hiding this comment.
Something that's now clear is that we only ever create one instance of the InternalLoggingChannelInterceptor and InternalLoggingServerInterceptor each and install those in the Global interceptors. So the factory is kind of useless. One thing we may consider (may be a separate PR) is to eliminate the Factories and replace those with those respective interceptors.
There was a problem hiding this comment.
How about a TODO comment saying we will eliminate the factory?
gcp-observability/src/main/java/io/grpc/gcp/observability/GcpObservability.java
Show resolved
Hide resolved
| @@ -40,7 +40,7 @@ | |||
| import java.util.logging.Logger; | |||
|
|
|||
| /** | |||
| * A logging interceptor for {@code LoggingChannelProvider}. | |||
| * A logging client interceptor for Observability. | |||
There was a problem hiding this comment.
As mentioned elsewhere we should consider getting rid of the Factory (in the Server interceptor as well). A separate PR seems more convenient?
There was a problem hiding this comment.
I do agree and will do this in another PR.
There was a problem hiding this comment.
Pls add a TODO comment saying so
gcp-observability/src/main/java/io/grpc/gcp/observability/logging/GcpLogSink.java
Outdated
Show resolved
Hide resolved
gcp-observability/src/main/java/io/grpc/gcp/observability/logging/GcpLogSink.java
Show resolved
Hide resolved
gcp-observability/src/main/java/io/grpc/gcp/observability/logging/GcpLogSink.java
Outdated
Show resolved
Hide resolved
gcp-observability/src/test/java/io/grpc/gcp/observability/GcpObservabilityTest.java
Outdated
Show resolved
Hide resolved
| * | ||
| * <p>Ignoring test, because it calls external Cloud Monitoring APIs. To test cloud monitoring | ||
| * setup locally, 1. Set up Cloud auth credentials 2. Assign permissions to service account to | ||
| * write metrics to project specified by variable PROJECT_ID 3. Comment @Ignore annotation |
There was a problem hiding this comment.
I am assuming when enabled with the correct set up this test passes? It might be worth mentioning that.
gcp-observability/src/test/java/io/grpc/gcp/observability/MetricsTest.java
Outdated
Show resolved
Hide resolved
gcp-observability/src/test/java/io/grpc/gcp/observability/TracesTest.java
Show resolved
Hide resolved
gcp-observability/src/test/java/io/grpc/gcp/observability/logging/GcpLogSinkTest.java
Outdated
Show resolved
Hide resolved
| Sink mockSink = new GcpLogSink(mockLogging, destProjectName, locationTags, | ||
| customTags, flushLimit); | ||
| assertThat(mockSink).isInstanceOf(GcpLogSink.class); | ||
| assertThat(spySink).isInstanceOf(GcpLogSink.class); |
There was a problem hiding this comment.
what does this test achieve? Or what does it really test from the "class-under-test"?
There was a problem hiding this comment.
Updated test to check if sink is instance of Sink interface
|
|
||
| ArgumentCaptor<Collection<LogEntry>> logEntrySetCaptor = ArgumentCaptor.forClass( | ||
| (Class) Collection.class); | ||
| verify(mockLogging, times(1)).write(logEntrySetCaptor.capture()); | ||
| for (Iterator<LogEntry> it = logEntrySetCaptor.getValue().iterator(); it.hasNext(); ) { | ||
| LogEntry entry = it.next(); | ||
| System.out.println(entry); | ||
| assertThat(entry.getPayload().getData()).isEqualTo(expectedStructLogProto); |
There was a problem hiding this comment.
Just noticed expectedStructLogProto - can it be defined in upper-case as EXPECTED_STRUCT_LOG_PROTO since it is like a constant object?
gcp-observability/src/test/java/io/grpc/gcp/observability/logging/GcpLogSinkTest.java
Outdated
Show resolved
Hide resolved
|
Addressed comments. PTAL |
| assertThat(mockSink).isInstanceOf(GcpLogSink.class); | ||
| GcpLogSink sink = new GcpLogSink(mockLogging, destProjectName, locationTags, | ||
| customTags, FLUSH_LIMIT); | ||
| assertThat(sink).isInstanceOf(Sink.class); |
There was a problem hiding this comment.
What's the value of this assert? Since GcpLogSink is declared to implement Sink this will be true so the test seems trivial - does not verify any run time behavior
There was a problem hiding this comment.
Makes sense. Removed the test since this was not adding any additional value.
gcp-observability/src/test/java/io/grpc/gcp/observability/logging/GcpLogSinkTest.java
Show resolved
Hide resolved
|
CC @ejona86 |
This PR removes
ManagedChannelProviderandServerProviderused by logging.With this change, going forward all three features of Observability namely logging, metrics and traces will use
GlobalInterceptorsto set Client/Server interceptors as well as Stream Tracers.CC @sanjaypujare