opentelemetry-go
opentelemetry-go copied to clipboard
[WIP / For Discussion] semconv: Add metric generation
Closes: #4528
This is a first attempt in generating some metric constants as a first step in completing deeper generation of semantic conventions for metrics. I borrowed a bit of the jinja logic from opentelemetry-python, as well as some of the formatting functions from the jinja template used in this repos semconv directory.
It needs some work, but this is a start to get a discussion going.
I added some more consts in this last commit. This would allow us to change the usage mentioned in #4528 like this:
https://github.com/open-telemetry/opentelemetry-go/blob/bd60b70c32d3af917c624254540b560ca9301116/semconv/v1.24.0/metric.go#L729-L737
from opentelemetry-go-contrib/blob/main/instrumentation/google.golang.org/grpc/otelgrpc/config.go:
// original
c.rpcDuration, err = c.meter.Float64Histogram("rpc."+role+".duration",
metric.WithDescription("Measures the duration of inbound RPC."),
metric.WithUnit("ms"))
// with generated consts
c.rpcDuration, err = c.meter.Float64Histogram(semconv.RPCServerDurationName,
metric.WithDescription(semconv.RPCServerDurationDescription),
metric.WithUnit(semconv.RPCServerDurationUnit))
@pellared suggested "at least the instrument name [could be generated], but maybe something that creates a whole instrument would be better? but it may be too complex". I think it's likely possible we can gather these items together, possibly in the jinja. Shouldn't be too complex. I want to get some feedback on the current implementation to make sure it's on track.
A few other notes for discussing:
- I noticed that the usage case here hardcodes
"Measures the duration of inbound RPC"even though theroleis a variable. (i.e., the message should be "Measures the duration of outbound RPC" whenrole == 'client'). - I came across
metric_groupas an option for identifying a specific type of semantic convention, but I don't see any definitions (outside of some test data in the build_tools repo) utilizing it. Likely a question for semantic conventions SIG group. - many of the metrics have attributes, and many of those attributes have
refs. I'm not sure if those should be considered as scope for this. Ex:
// SystemCPUTime is the metric conforming to the "system.cpu.time" semantic
// conventions. It represents the seconds each logical CPU spent on each mode
// Instrument: counter
// Unit: s
// Stability: None
// attribute: system.cpu.logical_number
// attribute: system.cpu.state
SystemCPUTimeName = attribute.Key("system.cpu.time")
SystemCPUTimeUnit = attribute.Key("s")
SystemCPUTimeDescription = attribute.Key("Seconds each logical CPU spent on each mode")
Examples of generated code and comments:
- defined
stabilityandbrief(notice theit_reps()output can be duplicative, adding "It represents..." to allbriefs:
// RPCServerDuration is the metric conforming to the "rpc.server.duration"
// semantic conventions. It represents the measures the duration of inbound
// RPC.
// Instrument: histogram
// Unit: ms
// Stability: Experimental
RPCServerDurationName = "rpc.server.duration"
RPCServerDurationUnit = "ms"
RPCServerDurationDescription = "Measures the duration of inbound RPC."
- when undefined
stability/briefin semantic-conventions:
// SystemFilesystemUsage is the metric conforming to the
// "system.filesystem.usage" semantic conventions.
// Instrument: updowncounter
// Unit: By
// Stability: Experimental
// NOTE: The description (brief) for this metric is not defined in the semantic-conventions repository.
SystemFilesystemUsageName = "system.filesystem.usage"
SystemFilesystemUsageUnit = "By"
Update:
I added some little string replacements to the jinja template for the metrics, but I believe a better approach would be to add "ASPNETCore" and "JVM" to the capitalizations slice in opentelemetry-go-build-tools. I rebuilt the semconvgen and it produced the desired(?) result (across all semantic conventions), for example:
+++ b/semconv/v1.24.0/attribute_group.go
@@ -284,7 +284,7 @@ func PoolName(val string) attribute.KeyValue {
// ASP.NET Core attributes
const (
- // AspnetcoreDiagnosticsHandlerTypeKey is the attribute Key conforming to
+ // ASPNETCoreDiagnosticsHandlerTypeKey is the attribute Key conforming to
// the "aspnetcore.diagnostics.handler.type" semantic conventions. It
// represents the full type name of the
// [`IExceptionHandler`](https://learn.microsoft.com/dotnet/api/microsoft.aspnetcore.diagnostics.iexceptionhandler)
@@ -295,9 +295,9 @@ const (
// was handled by this handler.)
// Stability: experimental
// Examples: 'Contoso.MyHandler'
- AspnetcoreDiagnosticsHandlerTypeKey = attribute.Key("aspnetcore.diagnostics.handler.type")
+ ASPNETCoreDiagnosticsHandlerTypeKey = attribute.Key("aspnetcore.diagnostics.handler.type")
This however, would be a breaking change, so we should discuss our options, and it might be better for now just to leave them as camel case.
This however, would be a breaking change, so we should discuss our options, and it might be better for now just to leave them as camel case.
We could change semconvgen so that it can accepts capitalizations from the user (probably via some file that can be select using e.g. via capitalizations-path flag).
We need to make sure that the current package semconv/v1.24.0 does not have breaking changes. But next package e.g. semconv/v1.25.0 can have these changes applied. Then we could use different capitalizations input when generating code.
Also I am fine, leaving names like JvmMemoryInitName for semconv/v1.24.0 given that attribute names have already bad capitalization and that these are non-Go specific semantic conventions.
I see four options at this point:
- Leave the PR as-is, limit the formatting logic of metric names to the metrics jinja template:
{% macro to_go_name(name) -%}
{%- if name.startswith('aspnetcore.') -%}
ASPNETCore{{ name[11:] | replace(".", " ") | replace("_", " ") | title | replace(" ", "") }}
{%- elif name.startswith('jvm.') -%}
JVM{{ name[4:] | replace(".", " ") | replace("_", " ") | title | replace(" ", "") }}
{%- else -%}
{{ name | replace(".", " ") | replace("_", " ") | title | replace(" ", "") }}
{%- endif -%}
{%- endmacro %}
- Revert the special handling altogether and keep the formatting as
JvmFooBarandAspnetcoreFooBar - Add
ASPNETCoreandJVMto thecapitalizationsslice in thesemconvgentool, which improves (?) readability across all generatedsemconv. - Modify the
semconvgentool to accept a custom capitalizations file (of type ?) to append to thecapitalizationsslice:
# ... in Makefile
$(SEMCONVGEN) -i "$(OTEL_SEMCONV_REPO)/model/." --only=metric -f metric.go \
-t "$(SEMCONVPKG)/metric_template.j2" \
--capitalizations-path=/path/to/metric-capitalizations.txt -s "$(TAG)"
The third option is the only one introducing breaking changes since metric generations are being introduced in this PR. @pellared (maybe @MadVikingGod too), if you have some guidance on which approach is best, I'd appreciate your insight.
I think we can start with option 2 and work on 4 concurrently so that we can use the new feature when we will be generating semconv/v1.25.0.
Ticket for adding the custom capitalizations flag to semconvgen: https://github.com/open-telemetry/opentelemetry-go-build-tools/issues/528
PR (WIP) for adding the flag and processing of custom capitalizations.
@open-telemetry/go-approvers PTAL
Codecov Report
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 83.7%. Comparing base (
35c9570) to head (04f11d0).
Additional details and impacted files
@@ Coverage Diff @@
## main #4880 +/- ##
=======================================
- Coverage 83.7% 83.7% -0.1%
=======================================
Files 252 252
Lines 16458 16458
=======================================
- Hits 13780 13778 -2
- Misses 2387 2389 +2
Partials 291 291