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 const
s 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 therole
is a variable. (i.e., the message should be "Measures the duration of outbound RPC" whenrole == 'client'
). - I came across
metric_group
as 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
ref
s. 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
stability
andbrief
(notice theit_reps()
output can be duplicative, adding "It represents..." to allbrief
s:
// 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
/brief
in 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
JvmFooBar
andAspnetcoreFooBar
- Add
ASPNETCore
andJVM
to thecapitalizations
slice in thesemconvgen
tool, which improves (?) readability across all generatedsemconv
. - Modify the
semconvgen
tool to accept a custom capitalizations file (of type ?) to append to thecapitalizations
slice:
# ... 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