[exporter/awsemfexporter] Add user agent string when EBS metrics exist #312
Conversation
Signed-off-by: Zhihong Lin <zhiholin@amazon.com>
| ms := ilms.At(j).Metrics() | ||
| for k := 0; k < ms.Len(); k++ { | ||
| metric := ms.At(k) | ||
| if strings.HasPrefix(metric.Name(), "node_diskio_ebs") { |
There was a problem hiding this comment.
nit: could you have a const for node_diskio_ebs
| @@ -75,11 +76,7 @@ func newEmfExporter(config *Config, set exporter.Settings) (*emfExporter, error) | |||
| collectorID: collectorIdentifier.String(), | |||
| pusherMap: map[cwlogs.StreamKey]cwlogs.Pusher{}, | |||
| processResourceLabels: func(map[string]string) {}, | |||
| } | |||
|
|
|||
| if config.IsAppSignalsEnabled() { | |||
There was a problem hiding this comment.
This is in upstream, so it will cause merge conflicts. Is there a way we can keep this in? Or at least make note that this diverges from upstream
There was a problem hiding this comment.
This code has to be moved because of this line
emf.svcStructuredLog.Handlers().Build.PushFrontNamed(userAgent.Handler())
In a previous PR We moved svcStructuredLog variable around which is what we use to access the useragent. It is not available in newEmfExporter function anymore. We never added back PushFrontNamed function anywhere else. This basically means we stopped emitting appsignal useragent since then.
| @@ -27,12 +28,15 @@ const ( | |||
|
|
|||
| // TODO: Available in semconv/v1.21.0+. Replace after collector dependency is v0.91.0+. | |||
| attributeTelemetryDistroVersion = "telemetry.distro.version" | |||
|
|
|||
| attributeEBS = "EBS" | |||
There was a problem hiding this comment.
kinda weird that we have this code under internal/appsignals when its not appsignals related
There was a problem hiding this comment.
I think better to just rename this folder to internal/useragent. Useragent related logic doesn't have to be AppSignal specific.
| @@ -87,6 +92,30 @@ func (ua *UserAgent) Process(labels map[string]string) { | |||
| } | |||
| } | |||
|
|
|||
| // ProcessMetrics checks metric names for specific patterns and updates user agent accordingly | |||
| func (ua *UserAgent) ProcessMetrics(metrics pmetric.Metrics) { | |||
| // Check if we've already detected NVME | |||
There was a problem hiding this comment.
Feel like we should have a separate util file for each metric we plan on identifying. In the future we may want to add a flag for GPU. It would also make it easier for merge conflicts.
Maybe have like exporter/awsemfexporter/internal/useragent_util.go with a func userAgentUtil.processNVMEMetrics(rms) that reads the resource metric and returns if it has been found or not. And we can keep track if it's found previously to not re-scan the metrics.
There was a problem hiding this comment.
I don't think this will cause a merge conflict since this folder doesn't exist on upstream: https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/exporter/awsemfexporter/internal/
| ) | ||
|
|
||
| type UserAgent struct { | ||
| mu sync.RWMutex | ||
| prebuiltStr string | ||
| cache *ttlcache.Cache[string, string] | ||
| metrics map[string]struct{} |
There was a problem hiding this comment.
nit: prob would rename this variable to featureFlags or something relating to appending to the feature section
| @@ -10,12 +10,14 @@ import ( | |||
|
|
|||
| "github.com/aws/aws-sdk-go/aws/request" | |||
| "github.com/stretchr/testify/assert" | |||
| "go.opentelemetry.io/collector/pdata/pmetric" | |||
| semconv "go.opentelemetry.io/collector/semconv/v1.18.0" | |||
| ) | |||
|
|
|||
| func TestUserAgent(t *testing.T) { | |||
There was a problem hiding this comment.
can we have a test just for the build() function to ensure it can build multiple feature flags in 1 section?
Description
Adds additional user agent if EBS metrics exist. This scrapes all metric names and stops the scrape once EBS metric has been found. This new process will only activate for enhanced container insights.
Testing
Unit Tests.
User Agent string