feat(gax): actionable errors logging in ApiTracer framework#4153
feat(gax): actionable errors logging in ApiTracer framework#4153westarle wants to merge 2 commits intogoogleapis:mainfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the observability of RPC calls within the GAX framework by implementing structured logging for "Actionable Errors." It provides detailed context for failed RPC attempts, making it easier to diagnose and understand issues at the application level, aligning with Application Centric Observability guidelines. The new logging is conditionally enabled via an environment variable and integrates seamlessly with existing tracing infrastructure. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
ba3c2cd to
865c099
Compare
There was a problem hiding this comment.
Code Review
This pull request introduces logging for actionable errors within the ApiTracer framework, which is a great step towards better observability. The implementation uses a decorator pattern with LoggingTracer and LoggingTracerFactory to intercept failed RPC attempts and log relevant context, gated by an environment variable. The changes are well-structured. I have a couple of suggestions to improve maintainability and test robustness.
I am having trouble creating individual review comments. Click here to see my feedback.
gax-java/gax/src/main/java/com/google/api/gax/tracing/LoggingTracer.java (186-190)
The string literals for logging context keys like "error.type" are hardcoded. To improve maintainability and prevent potential typos, it's a good practice to define them as private static final constants at the top of the class and use them here.
For example, you could add:
private static final String ERROR_TYPE_ATTRIBUTE = "error.type";
private static final String GCP_ERRORS_DOMAIN_ATTRIBUTE = "gcp.errors.domain";
private static final String GCP_ERRORS_METADATA_PREFIX = "gcp.errors.metadata.";And then use these constants in this block.
gax-java/gax/src/test/java/com/google/api/gax/logging/LoggingUtilsTest.java (120-137)
The testLogActionableError_success test is a bit weak as it only verifies that getLogger() and isInfoEnabled() are called. It doesn't confirm that the logging context and message are correctly passed to the underlying logging framework. To make the test more robust, you should also verify that addKeyValue is called with the expected key-value pairs from the context and that log is called with the correct message on the LoggingEventBuilder.
@Test
void testLogActionableError_success() {
LoggingUtils.setLoggingEnabled(true);
LoggerProvider loggerProvider = mock(LoggerProvider.class);
Logger logger = mock(Logger.class);
when(loggerProvider.getLogger()).thenReturn(logger);
when(logger.isInfoEnabled()).thenReturn(true);
org.slf4j.spi.LoggingEventBuilder eventBuilder = mock(org.slf4j.spi.LoggingEventBuilder.class);
when(logger.atInfo()).thenReturn(eventBuilder);
when(eventBuilder.addKeyValue(anyString(), any())).thenReturn(eventBuilder);
Map<String, Object> context = Collections.singletonMap("key", "value");
LoggingUtils.logActionableError(context, loggerProvider, "message");
verify(loggerProvider).getLogger();
verify(logger).isInfoEnabled();
verify(eventBuilder).addKeyValue("key", "value");
verify(eventBuilder).log("message");
}| if (apiTracerFactory instanceof SpanTracerFactory) { | ||
| apiTracerFactory = apiTracerFactory.withContext(apiTracerContext); | ||
| } | ||
| if (com.google.api.gax.logging.LoggingUtils.isLoggingEnabled()) { |
There was a problem hiding this comment.
We plan to introduce a CompositeTracer and CompositeTracerFactory that could contain a list of tracer/facrtories, similar to what is already done in Spanner. Then require customers to create a CompositeTracerFactory that contains SpanFactory/MetricTracerFactory and potentially LoggingTracerFactory.
Can we still rely on the existence of a LoggingTracerFactory to enable this feature?
| } | ||
| } | ||
|
|
||
| if (error instanceof ApiException) { |
This PR introduces SLF4J logging for "Actionable Errors" occurring at the T4 (RPC attempt) level, directly answering the Application Centric Observability guidelines.
Changes: