gcp-observability: remove logging channel/server providers#9424
gcp-observability: remove logging channel/server providers#9424sanjaypujare merged 5 commits intogrpc:masterfrom
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?
|
|
||
| /** | ||
| * 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
| * | ||
| * <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.
| 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
| 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?
sanjaypujare
left a comment
There was a problem hiding this comment.
finished 1st round. Will do the 2nd round once the comments are addressed
|
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.
sanjaypujare
left a comment
There was a problem hiding this comment.
Almost there. Small changes requested
|
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