feat: add support for FaaS telemetryapi metrics #2066
Conversation
herin049
force-pushed
the
feat/telemetryapi-metrics
branch
from
c03090e to
bed59f8
Compare
| "github.com/open-telemetry/opentelemetry-collector-contrib/pkg/pdatautil" | ||
| "go.opentelemetry.io/collector/pdata/pcommon" | ||
| "go.opentelemetry.io/collector/pdata/pmetric" | ||
| semconv2 "go.opentelemetry.io/otel/semconv/v1.24.0" |
There was a problem hiding this comment.
Why is this specific version needed? Can we not just use the same 1.25.0 version that is used elsewhere?
| "go.opentelemetry.io/collector/pdata/ptrace" | ||
| "go.opentelemetry.io/collector/receiver" | ||
| semconv "go.opentelemetry.io/collector/semconv/v1.25.0" | ||
| semconv2 "go.opentelemetry.io/otel/semconv/v1.24.0" |
There was a problem hiding this comment.
Related to other semconv version remark but I see now that you are seemingly doing this to deal with the deprecation of the old collector/semconv package in favor of otel/semconv. Could we not use "go.opentelemetry.io/otel/semconv/v1.25.0` for all semconv attributes? I'm assuming that that same version in the new package matches the one from the old package
| span.SetTraceID(newTraceID()) | ||
| span.SetSpanID(newSpanID()) | ||
| span.SetName("platform.initRuntimeDone") | ||
| span.SetName(fmt.Sprintf("init %s", r.faasName)) |
There was a problem hiding this comment.
Just a question, why is this needed for supporting telemetryapi metrics? Or is this just a general improvement/fix?
| resource pcommon.Resource | ||
| faasFunctionVersion string | ||
| faasName string | ||
| faaSMetricBuilders *FaaSMetricBuilders |
There was a problem hiding this comment.
debatble nit - naming
| faaSMetricBuilders *FaaSMetricBuilders | |
| faasMetricBuilders *FaasMetricBuilders |
lmk what you think.
| r.logger.Error("error receiving traces", zap.Error(err)) | ||
| // Metrics | ||
| if r.nextMetrics != nil { | ||
| if metrics, err := r.createMetrics(slice); err == nil { |
There was a problem hiding this comment.
Should we be worried about concurrent map writes here? Since createMetrics modifies the faaSMetricBuilders maps...
| if record, ok := el.Record.(map[string]any); ok { | ||
| functionName, _ := record["functionName"].(string) | ||
| if functionName != "" { | ||
| r.faasName = functionName |
There was a problem hiding this comment.
Same-ish remark here as: https://github.com/open-telemetry/opentelemetry-lambda/pull/2066/changes#r2637209797
Just wondering if there's any risk for concurrent writes here? Since this single telemetryAPIReceiver instance is shared across request goroutines.
wpessers
added
enhancement
2 participants
Adds support for FaaS telemetry API metrics.