feat(obs): introduce gpc.client.repo span attribute #4111
Conversation
diegomarquezp
commented
Feb 12, 2026
diegomarquezp
commented
- feat(obs): generate gapic.properties file with repo property
- feat: implement repo handling in gax
- test: add tests for repo property
- chore: speed up image building
- build: introduce repo property to generation config
- chore: update showcase module
Summary of ChangesHello @diegomarquezp, 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 observability for generated GAPIC clients by introducing a new Highlights
Changelog
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
|
![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 introduces a new gpc.client.repo span attribute for observability, propagating repository information from build configuration to tracing spans. However, a significant logic error exists where telemetry keys are swapped, leading to data corruption in audit logs. Furthermore, vulnerabilities related to insecure data handling (property injection and option injection) have been identified in the code processing repository information. There is also a high-severity bug in a build script and a medium-severity opportunity for code simplification using modern Java features.
| if [ -z "${repo}" ]; then | ||
| repo=$(repo) | ||
| fi |
There was a problem hiding this comment.
The script attempts to call a repo command if the --repo argument is not provided. This command is not defined within the script or its sourced utilities, which will likely lead to a "command not found" error during execution. If the intention is to have a default value, it should be set explicitly. If no default is desired, it should probably default to an empty string to avoid an error.
| if [ -z "${repo}" ]; then | |
| repo=$(repo) | |
| fi | |
| if [ -z "${repo}" ]; then | |
| repo="" | |
| fi |
| public static final String REPO_ATTRIBUTE = "gcp.client.language"; | ||
| public static final String LANGUAGE_ATTRIBUTE = "gcp.client.repo"; |
There was a problem hiding this comment.
The constants REPO_ATTRIBUTE and LANGUAGE_ATTRIBUTE are swapped. REPO_ATTRIBUTE should be "gcp.client.repo" and LANGUAGE_ATTRIBUTE should be "gcp.client.language". This misconfiguration leads to incorrect telemetry data, where the repository name is reported as the language and vice-versa, corrupting audit logs and monitoring dashboards.
| public static final String REPO_ATTRIBUTE = "gcp.client.language"; | |
| public static final String LANGUAGE_ATTRIBUTE = "gcp.client.repo"; | |
| public static final String REPO_ATTRIBUTE = "gcp.client.repo"; | |
| public static final String LANGUAGE_ATTRIBUTE = "gcp.client.language"; |
| JarEntry jarEntry = new JarEntry("src/main/resources/gapic.properties"); | ||
| try { | ||
| jos.putNextEntry(jarEntry); | ||
| jos.write(String.format("repo=%s\n", repo).getBytes(StandardCharsets.UTF_8)); |
There was a problem hiding this comment.
The writeGapicPropertiesFile method writes the repo string directly into a properties file using String.format("repo=%s\n", repo). If the repo string (which originates from user-supplied plugin arguments) contains a newline character, it can be used to inject arbitrary properties into the gapic.properties file. This could potentially influence the behavior of other components that read this configuration file.
| static Optional<String> parseRepo(CodeGeneratorRequest request) { | ||
| return parseConfigArgument(request.getParameter(), KEY_REPO); | ||
| } |
There was a problem hiding this comment.
The parseRepo method uses parseConfigArgument, which parses plugin arguments by splitting the input string by commas. If the repo value contains a comma, it will be split, potentially allowing an attacker to inject additional plugin flags or options (e.g., injecting ,metadata to enable metadata generation).
| private static void loadRepoFromProperties(Builder builder, @Nullable InputStream is) { | ||
| if (is == null) { | ||
| return; | ||
| } | ||
| try { | ||
| Properties properties = new Properties(); | ||
| properties.load(is); | ||
| builder.setRepo(properties.getProperty(REPO_KEY)); | ||
| } catch (IOException e) { | ||
| LOGGER.log(Level.WARNING, "Could not load gapic.properties", e); | ||
| } finally { | ||
| try { | ||
| is.close(); | ||
| } catch (IOException e) { | ||
| LOGGER.log(Level.WARNING, "Could not close gapic.properties stream", e); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The loadRepoFromProperties method can be simplified by using a try-with-resources statement. This is the modern and recommended way to handle resources like InputStream in Java, as it ensures the stream is closed correctly and makes the code more concise and readable.
private static void loadRepoFromProperties(Builder builder, @Nullable InputStream is) {
if (is == null) {
return;
}
try (InputStream inputStream = is) {
Properties properties = new Properties();
properties.load(inputStream);
builder.setRepo(properties.getProperty(REPO_KEY));
} catch (IOException e) {
LOGGER.log(Level.WARNING, "Could not load or close gapic.properties", e);
}
}…tracing-attr/repo
|
|
1 participant
