[exporter/splunkhecexporter] expose HTTPClientSettings on splunkhecexporter #16839
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
Foresight Summary
View More Details⭕ build-and-test-windows workflow has finished in 5 seconds (41 minutes 22 seconds less than
|
| Job | Failed Steps | Tests | |
|---|---|---|---|
| windows-unittest-matrix | - 🔗 | N/A | See Details |
| windows-unittest | - 🔗 | N/A | See Details |
✅ telemetrygen workflow has finished in 50 seconds (1 minute 35 seconds less than main branch avg.) and finished at 25th 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 3 seconds (1 minute 23 seconds less than main branch avg.) and finished at 25th 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 47 seconds and finished at 25th Jan, 2023.
| Job | Failed Steps | Tests | |
|---|---|---|---|
| changed files | - 🔗 | N/A | See Details |
| check-links | - 🔗 | N/A | See Details |
✅ changelog workflow has finished in 2 minutes 42 seconds and finished at 25th Jan, 2023.
| Job | Failed Steps | Tests | |
|---|---|---|---|
| changelog | - 🔗 | N/A | See Details |
✅ prometheus-compliance-tests workflow has finished in 11 minutes 2 seconds (⚠️ 3 minutes 27 seconds more than main branch avg.) and finished at 25th Jan, 2023.
| Job | Failed Steps | Tests | |
|---|---|---|---|
| prometheus-compliance-tests | - 🔗 | ✅ 21 ❌ 0 ⏭ 0 🔗 | See Details |
build-and-test workflow has finished in 8 minutes 59 seconds (43 minutes 9 seconds less than main branch avg.) and finished at 25th Jan, 2023.
| Job | Failed Steps | Tests | |
|---|---|---|---|
| correctness-metrics | N/A | ✅ 2 ❌ 0 ⏭ 0 🔗 | See Details |
| correctness-traces | N/A | ✅ 17 ❌ 0 ⏭ 0 🔗 | See Details |
| unittest-matrix (1.18, internal) | N/A | ✅ 561 ❌ 0 ⏭ 0 🔗 | See Details |
| unittest-matrix (1.19, internal) | N/A | ✅ 561 ❌ 0 ⏭ 0 🔗 | See Details |
| unittest-matrix (1.18, extension) | N/A | ✅ 528 ❌ 0 ⏭ 0 🔗 | See Details |
| unittest-matrix (1.19, extension) | N/A | ✅ 528 ❌ 0 ⏭ 0 🔗 | See Details |
| unittest-matrix (1.18, processor) | N/A | ✅ 1471 ❌ 0 ⏭ 0 🔗 | See Details |
| unittest-matrix (1.19, processor) | N/A | ✅ 1471 ❌ 0 ⏭ 0 🔗 | See Details |
| unittest-matrix (1.18, receiver-0) | N/A | ✅ 2567 ❌ 0 ⏭ 0 🔗 | See Details |
| unittest-matrix (1.19, receiver-0) | N/A | ✅ 2567 ❌ 0 ⏭ 0 🔗 | See Details |
| unittest-matrix (1.19, exporter) | N/A | ✅ 2411 ❌ 0 ⏭ 0 🔗 | See Details |
| unittest-matrix (1.18, receiver-1) | N/A | ✅ 1944 ❌ 0 ⏭ 0 🔗 | See Details |
| unittest-matrix (1.19, other) | N/A | ✅ 4631 ❌ 0 ⏭ 0 🔗 | See Details |
| unittest-matrix (1.18, exporter) | N/A | ✅ 2411 ❌ 0 ⏭ 0 🔗 | See Details |
| unittest-matrix (1.19, receiver-1) | N/A | ✅ 1944 ❌ 0 ⏭ 0 🔗 | See Details |
| integration-tests | N/A | ✅ 55 ❌ 0 ⏭ 0 🔗 | See Details |
| unittest-matrix (1.18, other) | N/A | ✅ 4631 ❌ 0 ⏭ 0 🔗 | See Details |
✅ load-tests workflow has finished in 19 minutes 27 seconds (⚠️ 5 minutes 9 seconds more 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 |
*You can configure Foresight comments in your organization settings page.
MovieStoreGuy
approved these changes
Dec 12, 2022
|
@dmitryax Do you have time to quick review for this enhancement? The change is not that much, and also comes from an another owner of this component. |
atoulme
force-pushed
the
splunkhecexporter_httpclientsettings
branch
from
555dda7 to
993b98b
Compare
dmitryax
reviewed
Dec 16, 2022
|
@atoulme once you get a chance, please rebase |
atoulme
force-pushed
the
splunkhecexporter_httpclientsettings
branch
from
993b98b to
bf8b664
Compare
|
I restarted this PR from scratch. I used the opportunity to clean up some constructs no longer needed with exporterhelper. |
atoulme
force-pushed
the
splunkhecexporter_httpclientsettings
branch
2 times, most recently
from
08c6204 to
0005065
Compare
|
Rebased after 0.69.0, needs a new review. |
atoulme
force-pushed
the
splunkhecexporter_httpclientsettings
branch
from
0005065 to
cf3a472
Compare
dmitryax
reviewed
Jan 12, 2023
|
OK I will break it down into smaller PRs. Thanks! |
|
Per code review, I have removed the errors and hopefully simplified the diff a bit more. Let me know. |
|
Please rebase |
atoulme
force-pushed
the
splunkhecexporter_httpclientsettings
branch
from
80d03f3 to
168a424
Compare
atoulme
force-pushed
the
splunkhecexporter_httpclientsettings
branch
from
168a424 to
7c4d0b4
Compare
atoulme
force-pushed
the
splunkhecexporter_httpclientsettings
branch
from
7c4d0b4 to
83c0266
Compare
|
Had one lint job time out, rerunning... |
|
@bogdandrutu rebased |
dmitryax
approved these changes
Jan 26, 2023
|
|
||
| func buildHTTPClient(config *Config, host component.Host, telemetrySettings component.TelemetrySettings) (*http.Client, error) { | ||
| // we handle compression explicitly. | ||
| config.HTTPClientSettings.Compression = "" |
There was a problem hiding this comment.
We shouldn't mutate the original config. We should make a copy instead. Can you please create an issue to follow up on this?
There was a problem hiding this comment.
ok. I think we do this in signalfx too. I'll make an issue. I guess those objects will eventually be immutable, is that right?
There was a problem hiding this comment.
Forcing them to be immutable would be pretty complicated, but as a rule we've been trying to not mutate the original user-supplied config
There was a problem hiding this comment.
OK, so would you agree a better fix is to validate the user doesn't set a compression algo then? That would take care of this, and might be more elegant.
There was a problem hiding this comment.
That would be better, but we also override MaxIdleConns and MaxIdleConnsPerHost
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.
Description:
Expose HTTPClientSettings on the config of the exporter
Link to tracking Issue:
#16838
Testing:
Unit tests.
Documentation:
Update to README with new documentation and deprecation of the MaxConnections setting.