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

Add metrics and resource attribute config options from vcentereceiver in docs and tests #5813

Merged
merged 7 commits into from
Nov 21, 2023

Conversation

marctc
Copy link
Contributor

@marctc marctc commented Nov 20, 2023

PR Description

After merging #5715 we identified a couple of things to be done in a follow up PR.
This PR adds the missing configurable fields from vcenter receiver to its documentations site.
It also adds case scenarios for config conversion.

PR Checklist

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

Comment on lines 187 to 188
require.Equal(t, false, otelArgs.Metrics.VcenterClusterCPUEffective.Enabled)
require.Equal(t, true, otelArgs.Metrics.VcenterClusterCPULimit.Enabled)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
require.Equal(t, false, otelArgs.Metrics.VcenterClusterCPUEffective.Enabled)
require.Equal(t, true, otelArgs.Metrics.VcenterClusterCPULimit.Enabled)
require.False(t, otelArgs.Metrics.VcenterClusterCPUEffective.Enabled)
require.True(t, otelArgs.Metrics.VcenterClusterCPULimit.Enabled)

I think this is clearer, but I'm happy if you leave it as it is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -92,6 +96,77 @@ isn't provided, TLS won't be used for connections to the server.

{{< docs/shared lookup="flow/reference/components/otelcol-tls-config-block.md" source="agent" version="<AGENT VERSION>" >}}

### metrics block

Name | Type | Description | Default | Required
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mind making metric in the Type column a hyperlink to #### metric block please?
BTW After we get consensus on #5764, I might need to rework this doc to be in line with that future desicion.

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.

LGTM. You could address the hyperlink comment and the other comments from #5715 in a separate PR. I think the docs now look pretty good for a first release 👌

@marctc marctc merged commit 6c028c0 into main Nov 21, 2023
11 checks passed
@marctc marctc deleted the vcenter_add_missing branch November 21, 2023 12:44
grafanabot pushed a commit that referenced this pull request Nov 21, 2023
… in docs and tests (#5813)

Co-authored-by: Paulin Todev <[email protected]>
(cherry picked from commit 6c028c0)
clayton-cornell pushed a commit that referenced this pull request Nov 21, 2023
… in docs and tests (#5813) (#5825)

Co-authored-by: Paulin Todev <[email protected]>
(cherry picked from commit 6c028c0)

Co-authored-by: Marc Tudurí <[email protected]>
@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
backport release-v0.38 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.

3 participants