Implement AppSignals on EKS and native EC2 #929
Conversation
Co-authored-by: Lisa Guo <[email protected]> Co-authored-by: Ping Xiang <[email protected]> Co-authored-by: nanzhenAWS <[email protected]>
Co-authored-by: Lisa Guo <[email protected]> Co-authored-by: Lisa Guo <[email protected]> Co-authored-by: Harry <[email protected]> Co-authored-by: Hyunsoo Kim <[email protected]>
Co-authored-by: Mengyi Zhou (bjrara) <[email protected]>
Co-authored-by: Github Action <[email protected]>
Co-authored-by: Github Action <[email protected]>
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #929 +/- ##
==========================================
+ Coverage 57.58% 62.40% +4.81%
==========================================
Files 370 357 -13
Lines 17548 18278 +730
==========================================
+ Hits 10105 11406 +1301
+ Misses 6848 6296 -552
+ Partials 595 576 -19
☔ View full report in Codecov by Sentry. |
| } | ||
|
|
||
| func (cfg *Config) Validate() error { | ||
| // TODO: validate those mandatory fields (if exist) in the config |
There was a problem hiding this comment.
Shouldn't we try to compile the globs here so we don't wait until runtime to panic?
There was a problem hiding this comment.
yes it's still a todo. @thpierce, will this be added later?
There was a problem hiding this comment.
I don't think we have any mandatory fields. So this TODO can be removed.
There was a problem hiding this comment.
Validation doesn't only check if any mandatory field is missing, it's also about whether the values customer set there are legal/valid. Is there any validation we want to put for the later?
There was a problem hiding this comment.
Like mentioned above, could we validate the globs so if they're invalid they are discovered at translation time?
plugins/processors/awsappsignals/internal/normalizer/attributesnormalizer.go
Outdated
Show resolved
Hide resolved
plugins/processors/awsappsignals/internal/resolver/attributesresolver.go
Show resolved
Hide resolved
| // Attributes are provided for each log and trace, but not at the metric level | ||
| // Need to process attributes for every data point within a metric. | ||
| func (ap *awsappsignalsprocessor) processMetricAttributes(ctx context.Context, m pmetric.Metric, resourceAttribes pcommon.Map) { | ||
|
|
||
| // This is a lot of repeated code, but since there is no single parent superclass | ||
| // between metric data types, we can't use polymorphism. |
There was a problem hiding this comment.
There isn't a single parent superclass, but you can still reduce the repeated code.
The functions within each RemoveIf are exactly the same. The metricsMutators for loop is exactly the same for each too.
There was a problem hiding this comment.
They look the same, but they don't. The datapoints are of different types.
There was a problem hiding this comment.
You can use interfaces/generics to get them to behave the same.
type DataPoints[T HasAttributes] interface {
At(i int) T
Len() int
}
type HasAttributes interface {
Attributes() pcommon.Map
}
func getAttributes[T HasAttributes](dps DataPoints[T]) []pcommon.Map {
var attributes []pcommon.Map
for i := 0; i < dps.Len(); i++ {
attributes = append(attributes, dps.At(i).Attributes())
}
return attributes
}
func Test(m pmetric.Metric) {
switch m.Type() {
case pmetric.MetricTypeGauge:
getAttributes[pmetric.NumberDataPoint](m.Gauge().DataPoints())
}
}…ibutes package in awsappsignals processor. Minor tweaks
There was a problem hiding this comment.
Move plugins/processors/awsappsignals to processor/awsappsignals. plugins is a legacy directory for telegraf. Going forward, all new components should be top level like receiver/adapter and extension/agenthealth
There was a problem hiding this comment.
I see, I put this in plugins since I saw the other processors in there. If we do this then we should add the /processor directory to be unit tested
plugins/processors/awsappsignals/internal/resolver/attributesresolver.go
Show resolved
Hide resolved
| "app_signals": { | ||
| "type": "object", | ||
| "properties": {}, | ||
| "additionalProperties": true |
There was a problem hiding this comment.
nit: If this should always be empty, then consider setting additionalProperties to false, so the schema validation will fail. This way there's less confusion about the configuration.
| @@ -4,6 +4,7 @@ | |||
| package awsxray | |||
|
|
|||
| import ( | |||
| _ "embed" | |||
Description of the issue
This is the implementation for app signals within the cloudwatch agent. Includes config translation of opentelemetry components to enable collection of Latency, Error, and Fault metrics (through emf) and traces.
Description of changes
app_signalsunderlogs.metrics_collectedandtraces.traces_collectedLicense
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Tests
Created a dev build and tested on EKS and EC2.
Requirements
Before commit the code, please do the following steps.
make fmtandmake fmt-shmake lint