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.

Sign up for GitHub

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

Jump to bottom

[feature] Solarwinds Extension #4

Merged
merged 43 commits into from
Dec 18, 2024
Merged

[feature] Solarwinds Extension #4

merged 43 commits into from
Dec 18, 2024

Conversation

david-sevcik
Copy link
Contributor

@david-sevcik david-sevcik commented Dec 9, 2024 •
edited
Loading

Description

Adds new solarwinds extension which is sending heartbeat at specific interval (30s) to signal liveliness in SWO. The heartbeat signal is an uptime metric sw.otelcol.uptime.
It's decorated with a collector name, which is configurable in the config.
It also take over endpoint configuration from the solarwinds exporter and provides this configuration to the exporter at runtime. Hence the exporter is now dependent on the extension and doesn't start without it.

Tech Notes

  • The new extension was added to the built distribution - should be available for evrybody which will use the Solarwinds OTEL Collector
  • The GO version was raised because if some new features used in the codebase
  • The configuration, readme and tests for exporter were updated to not check the removed configuration
    • Some genereted tested had to be skipped, because the exporter now requires the extension to run, but the generated tests cannot be configured to provide the extension to the tested exporter
  • There can be more solarwinds extension instances - if that's the case, the exporter needs the instance ID of specific extension to connect, otherwise it fails - it needs to know which extension instance to ask for the endpoint configuration
  • there are some small fixes of pipeline and in the code

@github-actions github-actions bot added the docker label Dec 9, 2024
david-sevcik
david-sevcik commented Dec 10, 2024
View reviewed changes
Makefile Show resolved Hide resolved
@adschr adschr self-assigned this Dec 12, 2024
@adschr
Copy link
Contributor

adschr commented Dec 12, 2024

I believe these changes are incoming anyway, being in draft at this point, but to be sure: can we please adjust the PR name and add a proper description on what is being added/what the changes are/hints to reviewers on what to give explicit feedback on?

PR template incoming soon to standardize all of this :).

adschr
adschr reviewed Dec 12, 2024
View reviewed changes
exporter/solarwindsexporter/factory.go Outdated
metricsExporter := newExporter(ctx, exporterCfg, settings, metricsExporterType)
metricsExporter, err := newExporter(exporterCfg, settings, metricsExporterType)
if err != nil {
return nil, err // TODO: Wrap.
Copy link
Contributor

Choose a reason for hiding this comment

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

just adding explicit comment to make sure the TODO is either resolved or comment removed

@david-sevcik david-sevcik marked this pull request as ready for review December 16, 2024 18:15
@david-sevcik david-sevcik requested a review from a team as a code owner December 16, 2024 18:15
simek-m
simek-m reviewed Dec 17, 2024
View reviewed changes
extension/solarwindsextension/README.md Outdated
[development]: https://github.com/open-telemetry/opentelemetry-collector/blob/main/docs/component-stability.md#development
<!-- end autogenerated section -->

SolarWinds Extension is required sidecar for the [Solarwinds Exporter](../../exporter/solarwindsexporter).
Copy link
Contributor

@simek-m simek-m Dec 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'd be cautious of the term "sidecar" here - it could be confusing to our users with a K8s background where it means a specific thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What other term do you suggest?

adschr
adschr previously approved these changes Dec 17, 2024
View reviewed changes
Copy link
Contributor

@adschr adschr left a comment

Choose a reason for hiding this comment

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

left some nits, in general LGTM. Let's proceed only after we validate the need for the insecure setting.

jirkajanecek
jirkajanecek previously approved these changes Dec 18, 2024
View reviewed changes
@simek-m simek-m merged commit 8a8542e into main Dec 18, 2024
6 checks passed
@simek-m simek-m deleted the NH-95440-solarwinds-extension branch December 18, 2024 11:44
@adschr adschr mentioned this pull request Jan 2, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simek-m simek-m simek-m left review comments

@adschr adschr adschr approved these changes

@jirkajanecek jirkajanecek jirkajanecek approved these changes

Assignees

@adschr adschr

Labels
docker documentation Improvements or additions to documentation
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@david-sevcik @adschr @simek-m @jirkajanecek