examples, agent instructions, and additional keys#8
Conversation
There was a problem hiding this comment.
Pull request overview
Updates the semconv module to support broader cross-service telemetry comparisons by adding additional client attribute keys and improving documentation around how the package should be used.
Changes:
- Add new client attribute keys for ISP and app.
- Expand
README.mdwith usage examples and reference links. - Add
AGENTS.mddescribing repository rules/structure and expected resource attributes.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| semconv.go | Adds client.isp and client.app attribute keys under Client Info. |
| README.md | Rewrites README to position the module as a standard+custom key source and adds usage examples. |
| AGENTS.md | Introduces contributor/agent guidance and documents expected resource attributes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ClientVersionKey attribute.Key = "client.version" | ||
| ClientPreferredRegionKey attribute.Key = "client.preferred_region" | ||
| ClientIsProKey attribute.Key = "client.is_pro" | ||
| ClientISPKey attribute.Key = "client.isp" |
There was a problem hiding this comment.
ClientISPKey introduces a new exported identifier with an initialism style that differs from nearby client keys (e.g., ClientAsnKey). Since these constants are part of the public API, pick one consistent initialism convention for client attributes (e.g., use ClientIspKey to match ClientAsnKey, or rename the existing client initialisms to match Go initialism casing) before this gets depended on across services.
| ClientISPKey attribute.Key = "client.isp" | |
| ClientIspKey attribute.Key = "client.isp" |
| Standard OTel keys may have convenience functions, but custom keys do not. | ||
|
|
||
| ```go | ||
| attrs := map[attribute.Key]attribute.Value{ | ||
| semconv.GeoCountryISOCodeKey: attribute.StringValue("US"), // standard | ||
| semconv.HostNameKey: attribute.StringValue("phost-abcd"), // standard | ||
| semconv.TrackNameKey: attribute.StringValue("nidoran"), // custom | ||
| semconv.ProxyProtocolKey: attribute.StringValue("HTTPS"), // custom | ||
| } | ||
| ``` | ||
|
|
||
| ```go | ||
| attrs := []attribute.KeyValue{ | ||
| semconv.GeoCountryISOCode("US"), | ||
| semconv.HostName("phost-abcd"), | ||
| attribute.KeyValue{ |
There was a problem hiding this comment.
The README examples/reference imply that standard OTel semconv keys/functions are available as semconv.GeoCountryISOCodeKey, semconv.HostNameKey, and semconv.GeoCountryISOCode(...). With the current implementation, the dot-import of go.opentelemetry.io/otel/semconv/... does not re-export those identifiers from this module, so these examples won't compile for consumers. Either update the README to instruct importing upstream OTel semconv for standard keys, or add explicit re-export/forwarding declarations in this package.
| This package defines the canonical attribute keys for all telemetry (metrics, | ||
| traces, logs) across Lantern services. It re-exports standard OTel semconv and | ||
| adds custom keys for Lantern-specific concepts. | ||
|
|
||
| ## Rules | ||
|
|
||
| 1. **Never hardcode attribute key strings.** Always use `semconv.*` constants. | ||
| 2. **Prefer standard OTel keys** when one exists for the concept. Only use | ||
| custom keys for Lantern-specific domains (proxies, routes, tracks, bandit, | ||
| clients, EIPs). | ||
| 3. **Align with existing usage.** Before adding a new key, check `semconv.go` | ||
| and grep across services for how similar resources are already described. | ||
| 4. **Naming matters for incident response.** These keys power dashboards and | ||
| alerts. Consistent naming lets on-call engineers query across services. | ||
|
|
||
| ## Structure | ||
|
|
||
| - `semconv.go` — all key definitions, grouped by domain | ||
| - Standard OTel keys are available via dot-import of | ||
| `go.opentelemetry.io/otel/semconv/v1.37.0` | ||
|
|
||
| ## Expected Resources | ||
|
|
||
| Every service must set these on its telemetry resource. Dashboards and alerts | ||
| depend on them for filtering and grouping during incident response. | ||
|
|
||
| - `semconv.ServiceNameKey` — identifies the service | ||
| - `semconv.ServiceVersionKey` — deployment error rate monitoring | ||
| - `semconv.DeploymentEnvironmentNameKey` — environment (prod/staging/dev) |
There was a problem hiding this comment.
This file states that the package "re-exports standard OTel semconv" and that services should set resources like semconv.ServiceNameKey / semconv.ServiceVersionKey. With the current Go implementation (dot-import in semconv.go), upstream semconv identifiers are not actually exported from this module, so references like semconv.ServiceNameKey won't be available to consumers. Either adjust this guidance to use go.opentelemetry.io/otel/semconv/... for standard keys, or implement explicit re-exports in this package to match the documented contract.
jay-418
changed the title
Has matching changes in lantern-box and http-proxy