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

[headerssetter] Extend headerssetter with header actions. #16818

Merged
merged 6 commits into from
Jan 26, 2023

Conversation

kovrus
Copy link
Member

@kovrus kovrus commented Dec 8, 2022

Description:

Add header modification actions to the headers setter extension.

Link to tracking Issue:
Resolves #16581
Resolves #7596

Testing:
Unit tests

Documentation:
Updated README

@runforesight
Copy link

runforesight bot commented Dec 8, 2022

Foresight Summary

    
Major Impacts

build-and-test-windows duration(4 seconds) has decreased 41 minutes 16 seconds compared to main branch avg(41 minutes 20 seconds).
View More Details

✅  telemetrygen workflow has finished in 59 seconds (1 minute 24 seconds less than main branch avg.) and finished at 26th Jan, 2023.


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

✅  tracegen workflow has finished in 1 minute 1 second (1 minute 23 seconds less than main branch avg.) and finished at 26th Jan, 2023.


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

✅  check-links workflow has finished in 1 minute 30 seconds and finished at 26th Jan, 2023.


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

✅  build-and-test workflow has finished in 35 minutes 44 seconds (16 minutes 18 seconds less than main branch avg.) and finished at 26th Jan, 2023.


Job Failed Steps Tests
unittest-matrix (1.19, internal) -     🔗  ✅ 561  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.18, internal) -     🔗  ✅ 561  ❌ 0  ⏭ 0    🔗 See Details
correctness-metrics -     🔗  ✅ 2  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.19, extension) -     🔗  ✅ 547  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.19, processor) -     🔗  ✅ 1471  ❌ 0  ⏭ 0    🔗 See Details
correctness-traces -     🔗  ✅ 17  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.18, extension) -     🔗  ✅ 547  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.18, processor) -     🔗  ✅ 1471  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.19, receiver-0) -     🔗  ✅ 2567  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.18, receiver-0) -     🔗  ✅ 2567  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.18, exporter) -     🔗  ✅ 2413  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.19, exporter) -     🔗  ✅ 2413  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.19, receiver-1) -     🔗  ✅ 1944  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.18, receiver-1) -     🔗  ✅ 1944  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.18, other) -     🔗  ✅ 4655  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.19, other) -     🔗  ✅ 4655  ❌ 0  ⏭ 0    🔗 See Details
integration-tests -     🔗  ✅ 55  ❌ 0  ⏭ 0    🔗 See Details
setup-environment -     🔗  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
check-collector-module-version -     🔗  N/A See Details
check-codeowners -     🔗  N/A See Details
checks -     🔗  N/A See Details
build-examples -     🔗  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

✅  prometheus-compliance-tests workflow has finished in 3 minutes 48 seconds (3 minutes 47 seconds less than main branch avg.) and finished at 26th Jan, 2023.


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

⭕  build-and-test-windows workflow has finished in 4 seconds (41 minutes 16 seconds less than main branch avg.) and finished at 26th Jan, 2023.


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

✅  load-tests workflow has finished in 8 minutes 17 seconds (5 minutes 53 seconds less than main branch avg.) and finished at 26th Jan, 2023.


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

✅  changelog workflow has finished in 1 minute 36 seconds and finished at 26th Jan, 2023.


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

🔎 See details on Foresight

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

@kovrus kovrus force-pushed the issue-16581 branch 2 times, most recently from c7c32fc to 4b1c5bb Compare December 8, 2022 14:12
@kovrus kovrus marked this pull request as ready for review December 8, 2022 14:12
@kovrus kovrus requested a review from a team as a code owner December 8, 2022 14:12
@kovrus kovrus force-pushed the issue-16581 branch 2 times, most recently from 8f84d1f to 9a56e7a Compare December 8, 2022 14:23
Copy link
Contributor

@atoulme atoulme left a comment

Choose a reason for hiding this comment

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

LGTM!

// Validate checks if the extension configuration is valid
func (cfg *Config) Validate() error {
if cfg.HeadersConfig == nil || len(cfg.HeadersConfig) == 0 {
return errMissingHeadersConfig
}
for _, header := range cfg.HeadersConfig {
if header.Action == "" {
Copy link
Member

Choose a reason for hiding this comment

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

This makes the extension backward incompatible. You can assume that all existing actions are "insert", and you'll be backward compatible.

Copy link
Member Author

@kovrus kovrus Dec 14, 2022

Choose a reason for hiding this comment

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

It is still alpha, so there was no intention to stay compatible. Therefore, I left a note in the change log about changing the existing config. Is it fine to go this way or shall we care about bwc for alpah as well? Also, I wanted to require the action explicitly without taking insert or upsert as a default action.

Copy link
Member Author

Choose a reason for hiding this comment

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

maybe then change_type: 'enhancement' should be breaking?

Copy link
Member

Choose a reason for hiding this comment

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

Can we do this in two steps then? The first is non-breaking, assuming that 'insert' is the default action, and will log a WARN if no action is defined, then on N+2, we fail if no action is provided.

This way, current users won't be caught by surprise when they just bump the collector version: we warned them via changelog and via a WARN log entry on their collectors for at least one version.

extension/headerssetterextension/README.md Show resolved Hide resolved
@github-actions
Copy link
Contributor

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

@kovrus
Copy link
Member Author

kovrus commented Jan 10, 2023

@jpkrohling can you take another look? I replied to some comments and updated the change log entry.

@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 Jan 25, 2023
Copy link
Member

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

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

LGTM, we need only to settle on the backward compatibility discussion.

// Validate checks if the extension configuration is valid
func (cfg *Config) Validate() error {
if cfg.HeadersConfig == nil || len(cfg.HeadersConfig) == 0 {
return errMissingHeadersConfig
}
for _, header := range cfg.HeadersConfig {
if header.Action == "" {
Copy link
Member

Choose a reason for hiding this comment

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

Can we do this in two steps then? The first is non-breaking, assuming that 'insert' is the default action, and will log a WARN if no action is defined, then on N+2, we fail if no action is provided.

This way, current users won't be caught by surprise when they just bump the collector version: we warned them via changelog and via a WARN log entry on their collectors for at least one version.

@github-actions github-actions bot removed the Stale label Jan 26, 2023
@kovrus kovrus force-pushed the issue-16581 branch 2 times, most recently from cc044c9 to 04d6f58 Compare January 26, 2023 11:31
@kovrus
Copy link
Member Author

kovrus commented Jan 26, 2023

Can we do this in two steps then? The first is non-breaking, assuming that 'insert' is the default action, and will log a WARN if no action is defined, then on N+2, we fail if no action is provided.

Made upsert the default action, so no breaking changes now. I'll follow up after a couple of releases with removing the default action.

@jpkrohling jpkrohling added the ready to merge Code review completed; ready to merge by maintainers label Jan 26, 2023
@jpkrohling jpkrohling merged commit 8fe7e79 into open-telemetry:main Jan 26, 2023
@kovrus kovrus deleted the issue-16581 branch January 26, 2023 18:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
extension/headerssetter ready to merge Code review completed; ready to merge by maintainers
Projects
None yet
3 participants