Skip to content

feat(attributes): expand grpc attributes#460

Open
lucas-zimerman wants to merge 8 commits into
getsentry:mainfrom
lucas-zimerman:lz/grpc/missint-attributes
Open

feat(attributes): expand grpc attributes#460
lucas-zimerman wants to merge 8 commits into
getsentry:mainfrom
lucas-zimerman:lz/grpc/missint-attributes

Conversation

@lucas-zimerman

@lucas-zimerman lucas-zimerman commented Jul 2, 2026

Copy link
Copy Markdown

Description

Adds attribute conventions for the Google gRPC rich error model (google.rpc.error_details.proto), under the grpc.error.* namespace:

  • grpc.error.error_info.{reason,domain,metadata.<key>}
  • grpc.error.bad_request.field_violations
  • grpc.error.retry_info.retry_delay_ms
  • grpc.error.debug_info.{detail,stack_entries}
  • grpc.error.precondition_failure.violations
  • grpc.error.resource_info.{resource_type,resource_name,description,owner}
  • grpc.error.quota_failure.violations

Field names and shapes were verified against google/rpc/error_details.proto (AIP-193 error model) for fidelity. Structured/repeated fields (e.g. field_violations, violations) are encoded as JSON-stringified array entries, following the existing mcp.request.argument.<key> precedent. Attributes that may carry user- or resource-identifying data (e.g. error_info.metadata.<key>, debug_info.detail, debug_info.stack_entries, precondition_failure.violations, resource_info.resource_name, resource_info.owner, quota_failure.violations) are documented as PII and gated behind sendDefaultPii or Sentry's dataCollection configuration.

Unblocks getsentry/sentry-dart#3791.

PR Checklist

  • I have run yarn test and verified that the tests pass.
  • I have run yarn generate to generate and format code and docs.

If an attribute was added:

  • The attribute is in a namespace (e.g. nextjs.function_id, not function_id)
  • I have used the correct value for apply_scrubbing (i.e. manual or auto. Use never only for values that should never be scrubbed such as IDs)

@github-actions

github-actions Bot commented Jul 2, 2026

Copy link
Copy Markdown

Semver Impact of This PR

🟡 Minor (new features)

📋 Changelog Preview

This is how your changes will appear in the changelog.
Entries from this PR are highlighted with a left border (blockquote style).


New Features ✨

Attributes

  • Expand grpc attributes by lucas-zimerman in #460
  • Add sentry.metric.source by klochek in #476
  • Add faas.id (deprecated) in favor of cloud.resource_id by andreiborza in #475
  • Add faas.execution (deprecated) in favor of faas.invocation_id by andreiborza in #473
  • Add messaging attributes by s1gr1d in #469
  • Add url.same_origin (deprecated) and http.request.same_origin (replacement) attributes by Lms24 in #456
  • Add db.response.status_code by s1gr1d in #462

Other

  • (ai) Add granular cost attributes for cache and reasoning by vgrozdanic in #461
  • (js) Add _BASE attribute key exports for dynamic attribute keys by Lms24 in #471

Internal Changes 🔧

  • (ci) Stop inheriting secrets for changelog-preview by tobias-wilfert in #463

🤖 This preview updates automatically when you update the PR.

@lucas-zimerman

Copy link
Copy Markdown
Author

I used claude to fix the attributes.py, not sure why when I generate it manually it formats the entire file
image

@lucas-zimerman lucas-zimerman marked this pull request as ready for review July 2, 2026 01:36
@lucas-zimerman lucas-zimerman requested review from a team, Lms24, cleptric, mjq and nsdeschenes as code owners July 2, 2026 01:36

@buenaflor buenaflor left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if we should nest under rpc, gRPC themselves use a top level grpc.* (at least for metrics)

https://grpc.io/docs/guides/opentelemetry-metrics/

@Lms24 Lms24 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Theoretically, we have the error namespace (also in OTel) which "defines the shared attributes used to report an error". But I get that the attributes here are very specific to GRPC, with some exceptions.

Also to confirm: We don't capture an exception in the SDK because we explicitly only want to record the error on the span? I assume because the error is not from the application but happened wherever the procedure was executed?

I'm curious on other reviewers opinions here. Should we stay GRPC-specific or would we prefer adding a bunch of specific attributes to the error namespace?

Comment thread model/attributes/rpc/rpc__grpc__error__debug_info__detail.json Outdated
lucas-zimerman and others added 2 commits July 3, 2026 19:41
Co-authored-by: Lukas Stracke <lukas.stracke@sentry.io>
…pace

gRPC's own OTel metrics conventions use a top-level grpc.* namespace,
so nest the new google.rpc error-detail attributes there instead of
under rpc.grpc.error.*, addressing review feedback on PR getsentry#460.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@lucas-zimerman

lucas-zimerman commented Jul 3, 2026

Copy link
Copy Markdown
Author

I'm not sure if we should nest under rpc, gRPC themselves use a top level grpc.* (at least for metrics)

https://grpc.io/docs/guides/opentelemetry-metrics/

renamed to grpc.* on f0c8eed

Except for rpc.grpc.status_code that already existed on sentry-conventions.

@lucas-zimerman

Copy link
Copy Markdown
Author

Theoretically, we have the error namespace (also in OTel) which "defines the shared attributes used to report an error". But I get that the attributes here are very specific to GRPC, with some exceptions.

Also to confirm: We don't capture an exception in the SDK because we explicitly only want to record the error on the span? I assume because the error is not from the application but happened wherever the procedure was executed?

I'm curious on other reviewers opinions here. Should we stay GRPC-specific or would we prefer adding a bunch of specific attributes to the error namespace?

OTel's error.* namespace is intentionally minimal: it's really just error.type (a low-cardinality classifier like an exception class name or domain code), plus a deprecated error.message. It's explicitly not meant to carry rich, structured payloads. The docs even call out that message-like fields cause "unbounded cardinality" problems on spans.

Also. Capturing an exception client-side felt low-value here. The client only has the serialized google.rpc error detail, while the real exception with full context typically already exists server-side (when that service is also Sentry-instrumented). Turning every non-OK status into a client-side exception would also add noise for cases that are expected/handled control flow rather than bugs. Worth documenting that reasoning, but happy to make it opt-in if reviewers want richer client-side capture for unattended/uninstrumented peers.

@lucas-zimerman lucas-zimerman requested review from Lms24 and buenaflor July 6, 2026 02:12

@Lms24 Lms24 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used claude to fix the attributes.py, not sure why when I generate it manually it formats the entire file

You need to use yarn generate to generate the file. It shouldn't reformat the entire file though. We recently switched to oxfmt. Maybe you need to run yarn install again (?)

I'm not sure if we should nest under rpc, gRPC themselves use a top level grpc.* (at least for metrics)

renamed to grpc.* on f0c8eed

So I researched this and I agree, we should just introduce the grpc namespace. For some context, I'm gonna write down what I learned here. The reason for going to the dedicated grpc namespace is that OTel deprecated a bunch of rpc.grpc.* attributes. Ideally they should be replaced with top-level rpc attributes. But in a case like gRPC rich errors, this isn't really applicable, since it's a gRPC-specific concept. In this case, the T-shaped signals spec recommends creating a new specialized attribute. There's also precedence for this with jsonrpc, where they already moved from rpc.jsonrpc.* to jsonrpc.*. So it seems like OTel just didn't (yet) see a need for a grpc namespace, but we can do so :)

Except for rpc.grpc.status_code that already existed on sentry-conventions.

We should deprecate this field in favour of rpc.response.status_code like OTel did. I'd appreciate it if you could take care of this, but please in a follow-up PR, since it's not directly related to this one.

Comment thread model/attributes/grpc/grpc__error__retry_info__retry_delay_in_ms.json Outdated

@Lms24 Lms24 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gonna approve since the remaining changes are minimal to kick off the 3-day period:

  • please update the PR description with the final attribute names
  • look at my comment regarding _ms vs. _in_ms
  • would appreciate the follow-up PR with the status code deprecation (see comment above)

lucas-zimerman and others added 2 commits July 6, 2026 22:25
Rename retry_delay_in_ms to retry_delay_ms per OTel naming guidance,
and mention dataCollection alongside sendDefaultPii on all PII-gated
grpc error attributes, not just debug_info.detail.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@lucas-zimerman

Copy link
Copy Markdown
Author

Gonna approve since the remaining changes are minimal to kick off the 3-day period:

* please update the PR description with the final attribute names

* look at my comment regarding `_ms` vs. `_in_ms`

* would appreciate the follow-up PR with the status code deprecation (see comment above)

PR description updated, also updated the _ms field. Happy to make a follow-up PR too.

Also, I can't merge this PR since I am an external contributor here

Comment on lines +7279 to +7284
export const GRPC_ERROR_ERROR_INFO_METADATA_KEY = 'grpc.error.error_info.metadata.<key>';

/**
* Type for {@link GRPC_ERROR_ERROR_INFO_METADATA_KEY} grpc.error.error_info.metadata.<key>
*/
export type GRPC_ERROR_ERROR_INFO_METADATA_KEY_TYPE = string;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The generated file attributes.ts is missing the GRPC_ERROR_ERROR_INFO_METADATA_KEY_BASE constant, which should be created for attributes with a dynamic suffix.
Severity: MEDIUM

Suggested Fix

The attributes.ts file was not correctly regenerated. Run the yarn generate command to update the file and include the missing GRPC_ERROR_ERROR_INFO_METADATA_KEY_BASE constant export. This will align it with the model definition and other dynamic attributes.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location: javascript/sentry-conventions/src/attributes.ts#L7279-L7284

Potential issue: The generator script is designed to create a `_BASE` constant for
attributes marked with `has_dynamic_suffix: true`, such as
`grpc.error.error_info.metadata.<key>`. However, the corresponding
`GRPC_ERROR_ERROR_INFO_METADATA_KEY_BASE` constant is missing from the generated
`attributes.ts` file. This is inconsistent with other dynamic attributes in the file.
Without this base constant, SDK consumers are forced to hardcode the base string
`'grpc.error.error_info.metadata'` to construct dynamic keys, undermining the utility of
the conventions library.

Also affects:

  • model/attributes/grpc/grpc__error__error_info__metadata__[key].json:1~19

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants