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(OTRM): Add validation for otel configuration in the opamp agent #1469

Merged
merged 4 commits into from
Mar 5, 2024

Conversation

rnishtala-sumo
Copy link
Contributor

@rnishtala-sumo rnishtala-sumo commented Feb 26, 2024

Add validation for otel configuration in the opamp agent. These changes cover syntax/semantic issues with the configuration. Reverting configuration after a runtime issue is not part of this change.

@@ -77,7 +77,7 @@ func (c *Config) Unmarshal(component *confmap.Conf) error {
}

c.Builder = factory.CreateDefaultConfig()
if err := subComponent.Unmarshal(c.Builder, confmap.WithErrorUnused()); err != nil {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

confmap version was bumped up.

@rnishtala-sumo rnishtala-sumo marked this pull request as ready for review February 27, 2024 17:10
@rnishtala-sumo rnishtala-sumo requested a review from a team as a code owner February 27, 2024 17:10
@rnishtala-sumo rnishtala-sumo changed the title feat(OTRM): Add config validation for otel config in the opamp agent feat(OTRM): Add validation for otel configuration in the opamp agent Feb 27, 2024
Copy link
Collaborator

@echlebek echlebek left a comment

Choose a reason for hiding this comment

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

Looks generally good, just a few cosmetic issues 👍

pkg/extension/opampextension/opamp_agent.go Outdated Show resolved Hide resolved
_, errValidate := otelcoltest.LoadConfigAndValidate(p, factories)
if errValidate != nil {
o.logger.Error("Validation Failed... %v", zap.Error(errValidate))
return fmt.Errorf("cannot validate config named %v", errValidate)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this error formatting correct? Seems like it should be something like Errorf("cannot validate config named %q: %s", name, err)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@echlebek does the above example make sense or would you like to see a change in the format?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see, that makes sense

pkg/extension/opampextension/opamp_agent.go Outdated Show resolved Hide resolved
@rnishtala-sumo rnishtala-sumo force-pushed the otrm-config-validation branch 4 times, most recently from ba3300d to 86e944b Compare February 28, 2024 14:07
@rnishtala-sumo
Copy link
Contributor Author

@echlebek @swiatekm-sumo I'm working with the backend team to finalize a list of components to validate/support in the opamp agent before going GA.
https://sumologic.atlassian.net/browse/SUMO-234777?focusedCommentId=529085

I could update this PR or ensure that this list gets updated incrementally before going GA with this.

@rnishtala-sumo rnishtala-sumo force-pushed the otrm-config-validation branch from 86e944b to 03c40d0 Compare March 4, 2024 15:36
@rnishtala-sumo rnishtala-sumo force-pushed the otrm-config-validation branch 2 times, most recently from 3b027d5 to 1b615da Compare March 5, 2024 17:51
@rnishtala-sumo rnishtala-sumo force-pushed the otrm-config-validation branch from 1b615da to c65f012 Compare March 5, 2024 18:23
@rnishtala-sumo rnishtala-sumo merged commit 683a389 into main Mar 5, 2024
30 checks passed
@rnishtala-sumo rnishtala-sumo deleted the otrm-config-validation branch March 5, 2024 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants