[DO NOT MERGE] Application Signals GA Tracking #1133
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bjrara
force-pushed
the
metric_schema_merge
branch
from
d000725 to
94b1f89
Compare
bjrara
force-pushed
the
metric_schema_merge
branch
from
94b1f89 to
6b24d0c
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1133 +/- ##
==========================================
+ Coverage 57.58% 65.43% +7.84%
==========================================
Files 370 357 -13
Lines 17548 18819 +1271
==========================================
+ Hits 10105 12314 +2209
+ Misses 6848 5901 -947
- Partials 595 604 +9 ☔ View full report in Codecov by Sentry. |
lisguo
reviewed
Apr 11, 2024
plugins/processors/awsappsignals/internal/attributes/attributes.go
Outdated
Show resolved
Hide resolved
plugins/processors/awsappsignals/internal/attributes/attributes.go
Outdated
Show resolved
Hide resolved
plugins/processors/awsappsignals/internal/attributes/attributes.go
Outdated
Show resolved
Hide resolved
plugins/processors/awsappsignals/internal/resolver/attributesresolver.go
Outdated
Show resolved
Hide resolved
bjrara
force-pushed
the
metric_schema_merge
branch
3 times, most recently
from
72dd500 to
567fe5c
Compare
mxiamxia
reviewed
Apr 15, 2024
plugins/processors/awsapplicationsignals/internal/normalizer/attributesnormalizer.go
Outdated
Show resolved
Hide resolved
mxiamxia
reviewed
Apr 15, 2024
plugins/processors/awsapplicationsignals/internal/normalizer/attributesnormalizer.go
Show resolved
Hide resolved
mxiamxia
reviewed
Apr 15, 2024
plugins/processors/awsapplicationsignals/internal/resolver/attributesresolver.go
Show resolved
Hide resolved
bjrara
force-pushed
the
metric_schema_merge
branch
5 times, most recently
from
a2a14b9 to
349ce06
Compare
lisguo
previously approved these changes
Apr 18, 2024
jefchien
reviewed
Apr 18, 2024
plugins/processors/awsapplicationsignals/internal/cardinalitycontrol/metrics_limiter_test.go
Outdated
Show resolved
Hide resolved
plugins/processors/awsapplicationsignals/internal/normalizer/attributesnormalizer.go
Show resolved
Hide resolved
plugins/processors/awsapplicationsignals/internal/normalizer/attributesnormalizer.go
Outdated
Show resolved
Hide resolved
plugins/processors/awsapplicationsignals/internal/normalizer/attributesnormalizer.go
Outdated
Show resolved
Hide resolved
plugins/processors/awsapplicationsignals/internal/normalizer/attributesnormalizer.go
Show resolved
Hide resolved
| } | ||
| } | ||
| } | ||
| if _, ok := attributes.Get(attr.AWSLocalEnvironment); !ok { |
There was a problem hiding this comment.
nit: This could be in the block above.
There was a problem hiding this comment.
The check is intentionally done outside the previous block to set a default environment wherever the default environment is not set.
plugins/processors/awsapplicationsignals/internal/resolver/attributesresolver.go
Outdated
Show resolved
Hide resolved
plugins/processors/awsapplicationsignals/internal/resolver/attributesresolver_test.go
Outdated
Show resolved
Hide resolved
Comment on lines
+159
to
+162
| case config.ModeEC2, config.ModeECS: | ||
| return appSignalsConfigGeneric | ||
| default: | ||
| return appSignalsConfigGeneric |
There was a problem hiding this comment.
What is the difference between these? Do we need to have the config.ModeEC2, config.ModeECS checks?
There was a problem hiding this comment.
For ECS, it's for putting a placeholder so that we don't miss anything we adding full support for the platform.
| @@ -10,6 +10,10 @@ const ( | |||
| ModeWithIRSA = "withIRSA" | |||
| ) | |||
|
|
|||
| const ( | |||
| ModeECS = "ECS" | |||
There was a problem hiding this comment.
If this is only going to be used in the EMF translator, then keep it there. Want to make sure we don't accidentally set the translator context to ECS since that would break a lot of things.
There was a problem hiding this comment.
It's used in EMF and applicationsignals translator.
bjrara
force-pushed
the
metric_schema_merge
branch
from
33c86c3 to
4ecf735
Compare
This reverts commit 622dc67.
bjrara
force-pushed
the
metric_schema_merge
branch
from
4ecf735 to
9ee1908
Compare
bjrara
force-pushed
the
metric_schema_merge
branch
from
c6a601e to
fe56259
Compare
|
Closed in favor of #1151 |
This was referenced May 10, 2024
bjrara
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Please DO NOT MERGE this PR.
Description of the issue
Previous, we introduced different metric schema on different platforms. However, this design involves two major defects:
Description of changes
In this PR, we
/aws/application-signals/data.HostedIn.withEnvironment.K8s.RemoteNamespacetoRemoteEnvironmentto make it consistent with localEnvironment.RemoteResource*dimensions.In addition, the
replacementmetric rewriting rule is also updated to allow adding new predefined Application Signals dimensions to the existing metric.This PR needs to work together with other PRs:
License
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Tests
Describe what tests you have done.
Requirements
Before commit the code, please do the following steps.
make fmtandmake fmt-shmake lint