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

feat(exporter/otelcol): support converting vcenterreceiver #6714

Merged

Conversation

hainenber
Copy link
Contributor

PR Description

Which issue(s) this PR fixes

Close #6459

Notes to the Reviewer

PR Checklist

Signed-off-by: hainenber <dotronghai96@gmail.com>
@hainenber hainenber changed the title feat(otelcol/exporter): support converting vcenterreceiver feat(exporter/otelcol): support converting vcenterreceiver Mar 17, 2024
Copy link
Contributor

@erikbaranowski erikbaranowski left a comment

Choose a reason for hiding this comment

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

Thanks for the PR 😄 . Looks solid and I've left a couple comments to finalize this. Let me know if you'd like me to work them, no pressure.

Endpoint: cfg.Endpoint,
Username: cfg.Username,
Password: rivertypes.Secret(cfg.Password),

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like we are missing the conversion of MetricsBuilderConfig here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added the converter but it looks like the defaults for agent's vcenter doesn't seem to be in sync with upstream, hence the full-blown River converted result.

These blocks with enabled = true should be omitted, right? Just wanna make sure that I understand the concept of upstream syncing correctly here

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch - I researched it and it made sense to commit the change I tested. The way nested structs were being handled in the component led to this weird behavior and it should be all set now. There is a slight divergence in defaults but it is not so bad anymore.

…bute + use helper for converting TLS setting

Signed-off-by: hainenber <dotronghai96@gmail.com>
@hainenber hainenber force-pushed the support-convert-otelcol-vcenterreceiver branch from 175f964 to 007998d Compare March 19, 2024 17:30
Signed-off-by: erikbaranowski <39704712+erikbaranowski@users.noreply.github.com>
Copy link
Contributor

@erikbaranowski erikbaranowski left a comment

Choose a reason for hiding this comment

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

Thank you!

@erikbaranowski erikbaranowski enabled auto-merge (squash) March 20, 2024 19:08
@erikbaranowski
Copy link
Contributor

I disabled automerge so this doesn't cause conflict with a bug fix we are working on. Once that is shipped I can merge it into this PR and get this merged.

@erikbaranowski erikbaranowski enabled auto-merge (squash) March 21, 2024 19:39
@erikbaranowski erikbaranowski merged commit 60fdd26 into grafana:main Mar 21, 2024
10 checks passed
@github-actions github-actions bot added the frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed. label Apr 21, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support converting vcenterreceiver
2 participants