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

flow: Add otelcol.receiver.vcenter component #5715

Merged
merged 24 commits into from
Nov 16, 2023
Merged

Conversation

marctc
Copy link
Contributor

@marctc marctc commented Nov 6, 2023

PR Description

This PR adds otelcol.receiver.vcenter which enables collection of metrics of vShere
machines.

PR Checklist

  • CHANGELOG.md updated
  • Documentation added
  • Tests updated
  • Config converters updated

@marctc marctc requested review from a team and clayton-cornell as code owners November 6, 2023 14:05
@marctc marctc requested a review from a team as a code owner November 6, 2023 14:07
@ptodev ptodev self-assigned this Nov 10, 2023
require.NoError(t, ctrl.WaitRunning(time.Second))
}

func TestArguments_UnmarshalRiver(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

For unmarshal tests, I personally prefer the table driven approach. It allows you to add lots of tests with minimal overhead.

For example, I'd use the map[string]interface{} style when some config structs are internal to OTel:
https://github.com/grafana/agent/blob/main/component/otelcol/processor/span/span_test.go

If none of the config structs are internal, I'd go for specifying the OTel structs directly:
https://github.com/grafana/agent/blob/main/component/otelcol/connector/servicegraph/servicegraph_test.go

@grafana grafana deleted a comment from ptodev Nov 13, 2023
@grafana grafana deleted a comment from ptodev Nov 13, 2023
Copy link
Contributor

@ptodev ptodev left a comment

Choose a reason for hiding this comment

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

Thanks you, I'm happy for the PR to be merged. I do think we need to address the currently open comments on a follow-up PR though.

Also, we need more tests in the follow-up PR:

  • tests for unmarshaling config
  • a test which starts a mock http server and sends a mock payload, so that we can test that all metrics and attributes are streamed ok

Comment on lines +192 to +195
type MetricsBuilderConfig struct {
Metrics MetricsConfig `river:"metrics,block,optional"`
ResourceAttributes ResourceAttributesConfig `river:"resource_attributes,block,optional"`
}
Copy link
Contributor

Choose a reason for hiding this comment

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

In the follow-up PR we also need to document these options in our docs.

@marctc marctc enabled auto-merge (squash) November 16, 2023 16:15
@marctc marctc merged commit 440d01d into main Nov 16, 2023
10 checks passed
@marctc marctc deleted the add_vcenter_receiver branch November 16, 2023 16:29
@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 Feb 21, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 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.

2 participants