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/awsemfexporter] Add ServiceName as a valid token replacement in log_stream_name #16531

Merged
merged 1 commit into from
Mar 16, 2023

Conversation

sky333999
Copy link
Contributor

Description: Add JobName as a valid token replacement in log_stream_name

Testing: None. Not sure if each token warrants a new unit test. Can add one if deemed required.

Documentation: Updated README.md

@sky333999 sky333999 requested a review from a team November 29, 2022 22:12
@github-actions github-actions bot added the exporter/awsemf awsemf exporter label Nov 29, 2022
@sky333999
Copy link
Contributor Author

Historically, is there a reason to use patternKeyToAttributeMap as an allow list?

Based on the name of the var, it looks like the primary purpose of patternKeyToAttributeMap to check for an alternate attribute when a token isnt found. For ex, look for k8s.node.name if NodeName is not found.

But currently its also serving a secondary purpose as an allow list that restricts the list of tokens that can be replaced only to the ones in this map (due to this)

So if a user specifies {foo} as a token and foo does exist as an attribute, it still wouldnt be replaced since its not allow-listed. Is this intentional?
If not, we could perhaps reorder the code to extract the pattern {%} and then do an initial pass to see if its present in the attributes and if not found, check if patternKeyToAttributeMap[pattern] is found.

@Aneurysm9
Copy link
Member

Where does JobName come from?

As for whether to use patternKeyToAttributeMap as an allow list, I think that is probably the safest approach to take. The metric data we're processing is untrusted user input in a sense and we should be careful to only allow it to be used in intended ways.

@sky333999
Copy link
Contributor Author

sky333999 commented Dec 2, 2022

Where does JobName come from?

In the context of a prometheus pipeline, its a relabel we plan on setting to update prometheus_job to job since our customers currently expect a label called job to be present in the emf logs and this is also used for the stream name. (prometheus_job itself is set by the prometheus receiver).

@sky333999
Copy link
Contributor Author

https://aws-otel.github.io/docs/getting-started/container-insights/ecs-prometheus shows an example of how prometheus_job is relabelled to job.

  metricstransform:
    transforms:
      - include: ".*" # Rename customized job label back to job
        match_type: regexp
        action: update
        operations:
          - label: prometheus_job # must match the value configured in ecs_observer
            new_label: job
            action: update_label

So the intention is to use this job label as the log stream name.

@github-actions
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Dec 20, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Jan 4, 2023

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions bot closed this Jan 4, 2023
@sky333999
Copy link
Contributor Author

I dont see the button to reopen so maybe it needs some privileges... @Aneurysm9 should we keep this open while you are looking into this?

@Aneurysm9 Aneurysm9 reopened this Jan 5, 2023
@runforesight
Copy link

runforesight bot commented Jan 5, 2023

Foresight Summary

    
Major Impacts

TestScraper ❌ failed 1 times in 5 runs (20% fail rate).
TestScraper/cannot_authenticate ❌ failed 1 times in 5 runs (20% fail rate).
TestScraper/invalid_endpoint ❌ failed 1 times in 5 runs (20% fail rate).
TestScraper/metrics_golden ❌ failed 1 times in 5 runs (20% fail rate).
TestScraper/metrics_golden_sftp ❌ failed 1 times in 5 runs (20% fail rate).
build-and-test-windows duration(5 seconds) has decreased 42 minutes 24 seconds compared to main branch avg(42 minutes 29 seconds).
View More Details

✅  tracegen workflow has finished in 2 minutes 2 seconds and finished at 12th Jan, 2023.


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

✅  telemetrygen workflow has finished in 1 minute 5 seconds (1 minute 58 seconds less than main branch avg.) and finished at 3rd Mar, 2023.


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

✅  check-links workflow has finished in 1 minute 31 seconds (56 seconds less than main branch avg.) and finished at 3rd Mar, 2023.


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

✅  prometheus-compliance-tests workflow has finished in 3 minutes 21 seconds (5 minutes 31 seconds less than main branch avg.) and finished at 3rd Mar, 2023.


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

✅  load-tests workflow has finished in 7 minutes 9 seconds (9 minutes 47 seconds less than main branch avg.) and finished at 3rd Mar, 2023.


Job Failed Steps Tests
loadtest (TestIdleMode) -     🔗  ✅ 1  ❌ 0  ⏭ 0    🔗 See Details
loadtest (TestTraceAttributesProcessor) -     🔗  ✅ 3  ❌ 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) -     🔗  ✅ 18  ❌ 0  ⏭ 0    🔗 See Details
setup-environment -     🔗  N/A See Details

✅  e2e-tests workflow has finished in 13 minutes 10 seconds (3 minutes 13 seconds less than main branch avg.) and finished at 3rd Mar, 2023.


Job Failed Steps Tests
kubernetes-test (v1.25.3) -     🔗  N/A See Details
kubernetes-test (v1.26.0) -     🔗  N/A See Details
kubernetes-test (v1.24.7) -     🔗  N/A See Details
kubernetes-test (v1.23.13) -     🔗  N/A See Details

✅  build-and-test workflow has finished in 33 minutes 12 seconds (36 minutes 44 seconds less than main branch avg.) and finished at 3rd Mar, 2023.


Job Failed Steps Tests
unittest-matrix (1.20, exporter) -     🔗  ✅ 2472  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.19, exporter) -     🔗  ✅ 2472  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.20, receiver-1) -     🔗  ✅ 1937  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.19, other) -     🔗  ✅ 4789  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.19, receiver-1) -     🔗  ✅ 1937  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.20, other) -     🔗  ✅ 4789  ❌ 0  ⏭ 0    🔗 See Details
unittest (1.20) -     🔗  N/A See Details
unittest (1.19) -     🔗  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

⭕  build-and-test-windows workflow has finished in 5 seconds (42 minutes 24 seconds less than main branch avg.) and finished at 13th Mar, 2023.


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

✅  changelog workflow has finished in 2 minutes 10 seconds and finished at 13th Mar, 2023.


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

🔎 See details on Foresight

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

@github-actions github-actions bot removed the Stale label Jan 7, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Feb 1, 2023

This PR was marked stale due to lack of activity. It will be closed in 14 days.

Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

@sky333999 please rebase to pick up the change to run tests against go1.20 and unblock CI.

@codeboten
Copy link
Contributor

Another ping on this @sky333999

@sky333999
Copy link
Contributor Author

Thanks for the reminder @codeboten. Ive rebased but looks like I have some failing tests now.
I dont actually see the failure logs and the tests failing seem unrelated to my change. Could you please rerun the failures to confirm if they are intermittent?

@codeboten
Copy link
Contributor

Huh... weird, let me close and re-open the PR to kick off the tests

@codeboten codeboten closed this Mar 3, 2023
@codeboten codeboten reopened this Mar 3, 2023
@sky333999
Copy link
Contributor Author

Thanks @codeboten . Looks better now.

@atoulme atoulme added the ready to merge Code review completed; ready to merge by maintainers label Mar 13, 2023
@codeboten codeboten changed the title [exporter/awsemfexporter] Add JobName as a valid token replacement in log_stream_name [exporter/awsemfexporter] Add ServiceName as a valid token replacement in log_stream_name Mar 16, 2023
@codeboten codeboten merged commit 897db04 into open-telemetry:main Mar 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
exporter/awsemf awsemf exporter ready to merge Code review completed; ready to merge by maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants