opentelemetry-go icon indicating copy to clipboard operation
opentelemetry-go copied to clipboard

[WIP / For Discussion] semconv: Add metric generation

Open carrbs opened this issue 1 year ago • 2 comments

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.

carrbs avatar Feb 02 '24 07:02 carrbs

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 the role is a variable. (i.e., the message should be "Measures the duration of outbound RPC" when role == '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 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")

carrbs avatar Feb 07 '24 00:02 carrbs

Examples of generated code and comments:

  • defined stability and brief (notice the it_reps() output can be duplicative, adding "It represents..." to all briefs:
	// 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."
	// 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"

carrbs avatar Feb 22 '24 22:02 carrbs

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.

carrbs avatar Mar 13 '24 23:03 carrbs

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.

pellared avatar Mar 14 '24 08:03 pellared

I see four options at this point:

  1. 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 %}
  1. Revert the special handling altogether and keep the formatting as JvmFooBar and AspnetcoreFooBar
  2. Add ASPNETCore and JVM to the capitalizations slice in the semconvgen tool, which improves (?) readability across all generated semconv.
  3. Modify the semconvgen tool to accept a custom capitalizations file (of type ?) to append to the capitalizations 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.

carrbs avatar Mar 15 '24 18:03 carrbs

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.

pellared avatar Mar 15 '24 19:03 pellared

Ticket for adding the custom capitalizations flag to semconvgen: https://github.com/open-telemetry/opentelemetry-go-build-tools/issues/528

carrbs avatar Mar 15 '24 22:03 carrbs

PR (WIP) for adding the flag and processing of custom capitalizations.

carrbs avatar Mar 16 '24 18:03 carrbs

@open-telemetry/go-approvers PTAL

pellared avatar Mar 27 '24 09:03 pellared

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

Impacted file tree graph

@@           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             

see 1 file with indirect coverage changes

codecov[bot] avatar Apr 04 '24 18:04 codecov[bot]