Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[exporter/awsxray] Adding Log Group Names to X-Ray Exporter Config #16859

Merged
merged 18 commits into from
Dec 28, 2022

Conversation

atshaw43
Copy link
Contributor

@atshaw43 atshaw43 commented Dec 12, 2022

Description: Adding log group names to X-Ray exporter YAML config.

Link to tracking Issue: #16939

Testing: Unit Tests

Documentation:
Updated README.md
Will also update https://aws-otel.github.io/docs/getting-started/x-ray

@atshaw43 atshaw43 requested review from a team and mx-psi December 12, 2022 17:49
@runforesight
Copy link

runforesight bot commented Dec 12, 2022

Foresight Summary

    
Major Impacts

TestConsumeLogsUnexpectedExporterType ❌ failed 24 times in 24 runs (100% fail rate).
TestConsumeTracesUnexpectedExporterType ❌ failed 24 times in 24 runs (100% fail rate).
TestLoadInvalidConfig_NoScrapers ❌ failed 24 times in 24 runs (100% fail rate).
build-and-test-windows duration(4 seconds) has decreased 39 minutes 55 seconds compared to main branch avg(39 minutes 59 seconds).
View More Details

⭕  build-and-test-windows workflow has finished in 4 seconds (39 minutes 53 seconds less than main branch avg.) and finished at 13th Dec, 2022.


Job Failed Steps Tests
windows-unittest-matrix -     🔗  N/A See Details
windows-unittest -     🔗  N/A See Details

✅  changelog workflow has finished in 1 minute 19 seconds (6 minutes 7 seconds less than main branch avg.) and finished at 13th Dec, 2022.


Job Failed Steps Tests
changelog -     🔗  N/A See Details

✅  check-links workflow has finished in 1 minute 24 seconds (1 minute 59 seconds less than main branch avg.) and finished at 13th Dec, 2022.


Job Failed Steps Tests
changed files -     🔗  N/A See Details
check-links -     🔗  N/A See Details

✅  tracegen workflow has finished in 1 minute 30 seconds (2 minutes 12 seconds less than main branch avg.) and finished at 13th Dec, 2022.


Job Failed Steps Tests
build-dev -     🔗  N/A See Details
publish-latest -     🔗  N/A See Details
publish-stable -     🔗  N/A See Details

✅  prometheus-compliance-tests workflow has finished in 3 minutes 17 seconds (6 minutes 13 seconds less than main branch avg.) and finished at 13th Dec, 2022.


Job Failed Steps Tests
prometheus-compliance-tests -     🔗  ✅ 21  ❌ 0  ⏭ 0    🔗 See Details

✅  build-and-test workflow has finished in 36 minutes 32 seconds (24 minutes 50 seconds less than main branch avg.) and finished at 13th Dec, 2022. There are 6 test failures.


Job Failed Steps Tests
unittest-matrix (1.19, internal) -     🔗  ✅ 592  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.18, internal) -     🔗  ✅ 592  ❌ 0  ⏭ 0    🔗 See Details
correctness-metrics -     🔗  ✅ 2  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.18, processor) -     🔗  ✅ 1465  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.19, processor) -     🔗  ✅ 1465  ❌ 0  ⏭ 0    🔗 See Details
correctness-traces -     🔗  ✅ 17  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.19, extension) -     🔗  ✅ 528  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.18, extension) -     🔗  ✅ 528  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.18, receiver-0) -     🔗  ✅ 2532  ❌ 1  ⏭ 0    🔗 See Details
unittest-matrix (1.19, receiver-0) -     🔗  ✅ 2532  ❌ 1  ⏭ 0    🔗 See Details
unittest-matrix (1.19, other) -     🔗  ✅ 4363  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.19, exporter) -     🔗  ✅ 2240  ❌ 2  ⏭ 0    🔗 See Details
unittest-matrix (1.18, other) -     🔗  ✅ 4363  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.18, exporter) -     🔗  ✅ 2240  ❌ 2  ⏭ 0    🔗 See Details
unittest-matrix (1.18, receiver-1) -     🔗  ✅ 1854  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.19, receiver-1) -     🔗  ✅ 1854  ❌ 0  ⏭ 0    🔗 See Details
integration-tests -     🔗  ✅ 53  ❌ 0  ⏭ 0    🔗 See Details
setup-environment -     🔗  N/A See Details
checks -     🔗  N/A See Details
check-collector-module-version -     🔗  N/A See Details
check-codeowners -     🔗  N/A See Details
build-examples -     🔗  N/A See Details
lint-matrix (receiver-0) -     🔗  N/A See Details
lint-matrix (receiver-1) -     🔗  N/A See Details
lint-matrix (processor) -     🔗  N/A See Details
lint-matrix (exporter) -     🔗  N/A See Details
lint-matrix (extension) -     🔗  N/A See Details
lint-matrix (internal) -     🔗  N/A See Details
lint-matrix (other) -     🔗  N/A See Details
lint -     🔗  N/A See Details
unittest (1.19) -     🔗  N/A See Details
unittest (1.18) -     🔗  N/A See Details
cross-compile (darwin, amd64) -     🔗  N/A See Details
cross-compile (darwin, arm64) -     🔗  N/A See Details
cross-compile (linux, 386) -     🔗  N/A See Details
cross-compile (linux, amd64) -     🔗  N/A See Details
cross-compile (linux, arm) -     🔗  N/A See Details
cross-compile (linux, arm64) -     🔗  N/A See Details
cross-compile (linux, ppc64le) -     🔗  N/A See Details
cross-compile (windows, 386) -     🔗  N/A See Details
cross-compile (windows, amd64) -     🔗  N/A See Details
build-package (deb) -     🔗  N/A See Details
build-package (rpm) -     🔗  N/A See Details
windows-msi -     🔗  N/A See Details
publish-check -     🔗  N/A See Details
publish-dev -     🔗  N/A See Details
publish-stable -     🔗  N/A See Details

✅  load-tests workflow has finished in 8 minutes 28 seconds (8 minutes 1 second less than main branch avg.) and finished at 13th Dec, 2022.


Job Failed Steps Tests
loadtest (TestTraceAttributesProcessor) -     🔗  ✅ 3  ❌ 0  ⏭ 0    🔗 See Details
loadtest (TestIdleMode) -     🔗  ✅ 1  ❌ 0  ⏭ 0    🔗 See Details
loadtest (TestMetric10kDPS|TestMetricsFromFile) -     🔗  ✅ 6  ❌ 0  ⏭ 0    🔗 See Details
loadtest (TestTraceNoBackend10kSPS|TestTrace1kSPSWithAttrs) -     🔗  ✅ 8  ❌ 0  ⏭ 0    🔗 See Details
loadtest (TestMetricResourceProcessor|TestTrace10kSPS) -     🔗  ✅ 12  ❌ 0  ⏭ 0    🔗 See Details
loadtest (TestTraceBallast1kSPSWithAttrs|TestTraceBallast1kSPSAddAttrs) -     🔗  ✅ 10  ❌ 0  ⏭ 0    🔗 See Details
loadtest (TestBallastMemory|TestLog10kDPS) -     🔗  ✅ 19  ❌ 0  ⏭ 0    🔗 See Details
setup-environment -     🔗  N/A See Details

🔎 See details on Foresight

*You can configure Foresight comments in your organization settings page.

@github-actions github-actions bot requested a review from willarmiros December 12, 2022 17:50
@atshaw43 atshaw43 changed the title Adding Log Group Names to X-Ray Exporter Config [exporter/config]Adding Log Group Names to X-Ray Exporter Config Dec 12, 2022
@atshaw43 atshaw43 changed the title [exporter/config]Adding Log Group Names to X-Ray Exporter Config [exporter/config] Adding Log Group Names to X-Ray Exporter Config Dec 12, 2022
@atshaw43 atshaw43 changed the title [exporter/config] Adding Log Group Names to X-Ray Exporter Config [exporter/awsxray] Adding Log Group Names to X-Ray Exporter Config Dec 12, 2022
@wangzlei
Copy link
Contributor

wangzlei commented Dec 19, 2022

Can we have a xray segment example either in PR description for helping reviewer to understand

@atshaw43
Copy link
Contributor Author

The new field is aws_log_groups in the exporter YAML config for AWS. It is a set of cloud watch log group names.
Example config.:

awsxray:
awsxray/customname:
region: eu-west-1
resource_arn: "arn:aws:ec2:us-east1:123456789:instance/i-293hiuhe0u"
role_arn: "arn:aws:iam::123456789:role/monitoring-EKS-NodeInstanceRole"
indexed_attributes: [ "indexed_attr_0", "indexed_attr_1" ]
aws_log_groups: ["myLogGroup"]

This makes its way to cwl variable (Cloud Watch Logs) in aws.go. We use it as the lowest priority compared to the resource attributes.

@mx-psi
Copy link
Member

mx-psi commented Dec 20, 2022

@willarmiros could you review as a codeowner?

Copy link
Contributor

@willarmiros willarmiros left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wangzlei is in my organization, so I will approve based on his approval.

@@ -67,8 +67,8 @@ var (
)

// MakeSegmentDocumentString converts an OpenTelemetry Span to an X-Ray Segment and then serialzies to JSON
func MakeSegmentDocumentString(span ptrace.Span, resource pcommon.Resource, indexedAttrs []string, indexAllAttrs bool) (string, error) {
segment, err := MakeSegment(span, resource, indexedAttrs, indexAllAttrs)
func MakeSegmentDocumentString(span ptrace.Span, resource pcommon.Resource, indexedAttrs []string, indexAllAttrs bool, logGroupNames []string) (string, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly a question for @mx-psi or the OTel maintainer who's reviewing: is there a risk of a breaking change with changing the signature of this as a public method? I don't think so since it's part of the collector rather than an SDK, but want to confirm

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mx-psi
Bumping this question.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, sorry, missed this.

is there a risk of a breaking change with changing the signature of this as a public method? I don't think so since it's part of the collector rather than an SDK, but want to confirm

This is part of an internal package, so it is not part of the public API and thus there is no risk.

In general, we do care about the Go API of the Collector: we aim for Collector components to be eventually stable in both their usage as part of a Collector binary and as a library, but in this case this is fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants