fix:Use JSON string instead of dic for span attributes when content capture is disabled #4101
Conversation
…apture is disabled
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Summary of ChangesHello @DineshThumma9, 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 resolves a critical bug in the telemetry system where disabled content capture led to invalid OpenTelemetry span attributes. By switching from Python dictionaries to JSON strings for empty attribute values, the system now adheres to OpenTelemetry standards, preventing errors and ensuring that traces are correctly exported and visible in the Vertex AI Agent Engine. The change is focused on maintaining data integrity and system stability under specific configuration settings. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
adk-bot
added
the
tracing
|
Response from ADK Triaging Agent Hello @DineshThumma9, thank you for your contribution! Our records indicate that you haven't signed the Contributor License Agreement (CLA) yet. Please visit https://cla.developers.google.com/ to sign it. Once the CLA is signed, we can proceed with reviewing your PR. Thanks! |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly fixes a critical issue where using a Python dictionary for an OpenTelemetry span attribute caused crashes when content capture was disabled. The fix of using a JSON string '{}' is appropriate and well-tested. The updated unit tests are also a significant improvement in clarity and correctness. I've added a couple of suggestions to improve maintainability by using a constant for the empty JSON string and by refactoring duplicated test assertion logic into a helper function.
src/google/adk/telemetry/tracing.py
Outdated
| ) | ||
| else: | ||
| span.set_attribute('gcp.vertex.agent.tool_call_args', {}) | ||
| span.set_attribute('gcp.vertex.agent.tool_call_args', '{}') |
There was a problem hiding this comment.
To improve maintainability and avoid using a magic string, consider defining a constant for '{}' at the module level and reusing it here and in other similar places.
For example:
EMPTY_JSON_STRING = '{}'
...
span.set_attribute('gcp.vertex.agent.tool_call_args', EMPTY_JSON_STRING)This change would apply to all 6 locations modified in this PR (lines 152, 182, 222, 268, 293, 349) and could also replace other existing instances of the '{}' literal in this file for consistency.
| llm_request_calls = [ | ||
| call | ||
| for call in mock_span_fixture.set_attribute.call_args_list | ||
| if call.args[0] == 'gcp.vertex.agent.llm_request' | ||
| ] | ||
| assert ( | ||
| len(llm_request_calls) == 1 | ||
| ), "Expected 'gcp.vertex.agent.llm_request' to be set exactly once" | ||
| assert llm_request_calls[0].args[1] == '{}', ( | ||
| "Expected JSON string '{}' for llm_request when content capture is" | ||
| f' disabled, got {llm_request_calls[0].args[1]!r}' | ||
| ) | ||
|
|
||
| llm_response_calls = [ | ||
| call | ||
| for call in mock_span_fixture.set_attribute.call_args_list | ||
| if call.args[0] == 'gcp.vertex.agent.llm_response' | ||
| ] | ||
| assert ( | ||
| len(llm_response_calls) == 1 | ||
| ), "Expected 'gcp.vertex.agent.llm_response' to be set exactly once" | ||
| assert llm_response_calls[0].args[1] == '{}', ( | ||
| "Expected JSON string '{}' for llm_response when content capture is" | ||
| f' disabled, got {llm_response_calls[0].args[1]!r}' | ||
| ) |
There was a problem hiding this comment.
The new assertions are much clearer than the previous ones. To further improve the tests and reduce code duplication, you could extract this assertion logic into a helper function. This pattern is repeated for llm_request and llm_response here, and also in test_trace_tool_call_disabling_request_response_content and test_trace_merged_tool_disabling_request_response_content.
A helper could look something like this:
def _assert_span_attribute_set_to_empty_json(mock_span, attribute_name: str):
calls = [
call
for call in mock_span.set_attribute.call_args_list
if call.args[0] == attribute_name
]
assert (
len(calls) == 1
), f"Expected '{attribute_name}' to be set exactly once"
assert calls[0].args[1] == '{}', (
f"Expected JSON string '{{}}' for {attribute_name} when content capture is"
f' disabled, got {calls[0].args[1]!r}'
)
# Then in your test:
_assert_span_attribute_set_to_empty_json(mock_span_fixture, 'gcp.vertex.agent.llm_request')
_assert_span_attribute_set_to_empty_json(mock_span_fixture, 'gcp.vertex.agent.llm_response')This would make the tests more concise and easier to maintain.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This is an excellent pull request that addresses a critical bug causing crashes when telemetry content capture is disabled. The solution of replacing empty Python dictionaries with an empty JSON string for span attributes is correct and effectively resolves the issue. The introduction of the EMPTY_JSON_STRING constant is a great improvement for code maintainability and consistency. Furthermore, the unit tests have been thoughtfully updated with a new helper function, which not only verifies the fix but also enhances the readability and robustness of the test suite. The changes are well-executed and thoroughly tested.
|
Hi @DineshThumma9 , Thank you for your contribution! We appreciate you taking the time to submit this pull request. |
ryanaiagent
added
the
request clarification
I fixed linting now procced with review |
3 participants
Please ensure you have read the contribution guide before creating a pull request.
Link to Issue or Description of Change
1. Link to an existing issue (if applicable):
2. Or, if no issue exists, describe the change:
If applicable, please follow the issue templates to provide as much detail as possible.
Problem:
When
ADK_CAPTURE_MESSAGE_CONTENT_IN_SPANSis set tofalse, the ADK telemetry code was setting span attributes to empty Python dicts ({}) instead of valid OpenTelemetry attribute types.OpenTelemetry span attributes can only be primitives (
bool,str,bytes,int,float) or sequences of those types. Setting adictas an attribute value caused:Invalid type dict for attribute 'gcp.vertex.agent.llm_response' value...opentelemetry-exporter-gcp-loggingwith:ValueError: Unexpected typeSolution:
Changed all 6 instances of
{}(Python dict) to'{}'(JSON string) intracing.py. Strings are valid OpenTelemetry primitive types and correctly represent empty JSON objects.Affected locations:
trace_tool_call– lines 152, 182trace_merged_tool_calls– line 223trace_call_llm– lines 268, 293trace_send_data– line 349Testing Plan
Unit Tests:
Please include a summary of passed
pytestresults.pytest tests/unittests/telemetry/ -v # Result: 29 passed, 4 warnings in 9.66sUpdated 3 test functions to verify span attributes use JSON string
'{}'instead of dict{}:test_call_llm_disabling_request_response_contenttest_trace_tool_call_disabling_request_response_contenttest_trace_merged_tool_disabling_request_response_contentManual End-to-End (E2E) Tests:
Please provide instructions on how to manually test your changes, including any necessary setup or configuration. Please provide logs or screenshots to help reviewers better understand the fix.
Setup:
Result:
Invalid type dictwarningsVerified span attributes:
gcp.vertex.agent.tool_call_args:str = '{}'gcp.vertex.agent.tool_response:str = '{}'gcp.vertex.agent.llm_request:str = '{}'gcp.vertex.agent.llm_response:str = '{}'Checklist
Additional context
This fix is critical for telemetry to work correctly in Vertex AI Agent Engine deployments where sensitive content capture is disabled. The change is minimal (6 characters changed across 6 lines) and low-risk, affecting only the literal values used for empty content placeholders.
Files changed:
tracing.py– 6 lines (dict → string)test_spans.py– 3 test functions updated