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

metadata: add configuration_options for varnish and add validation #565

Merged
merged 2 commits into from
Apr 28, 2022

Conversation

ridwanmsharif
Copy link
Contributor

This change adds in the configuration option for Varnish and adds validation enforcement for new receivers.

@ridwanmsharif ridwanmsharif force-pushed the ridwanmsharif/fix-config-options branch 2 times, most recently from 6697ef1 to 3ba5285 Compare April 28, 2022 16:21
@@ -1,5 +1,6 @@
short_name: varnish
long_name: Varnish HTTP Cache
supported_app_version: ["6.0+", "7.0+"]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JonathanWamsley Could you confirm that this is the correct supported version for Varnish?

Copy link
Contributor

Choose a reason for hiding this comment

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

We don't use + but rather x to indicate all the versions in the series. So ["6.x", "7.0.x"] like in the scope doc. This is is what other receivers like wildfly does too.

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.

Comment on lines 383 to 388
// Validate the metadata.
validate := validator.New()
if err = validate.Struct(metadata); err != nil {
return nonRetryable, err
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I am also working on this :) It needs to do quite much to get it work in the we intended it to be.😮‍💨
metadata parsed by unmarshallstrict will allocate any substruct as {} instead of nill thus the required tag won't work. I will send a PR soon. For now you can restore this file back, i will handle the work in my PR :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Here is the PR: #567

@@ -1,5 +1,6 @@
short_name: varnish
long_name: Varnish HTTP Cache
Copy link
Contributor

Choose a reason for hiding this comment

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

It also needs Description from the scope doc, it will start failing the test once #567 is in

The Varnish integration collects cache and session metrics. It monitors the number of objects entering and exiting the cache, as well as the number of sessions and backend connections. The integration also collects Varnish access logs and parses them into a standardized json payload.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

@ridwanmsharif ridwanmsharif force-pushed the ridwanmsharif/fix-config-options branch from 3ba5285 to f1d3d55 Compare April 28, 2022 19:51
@ridwanmsharif ridwanmsharif merged commit 9645814 into master Apr 28, 2022
@ridwanmsharif ridwanmsharif deleted the ridwanmsharif/fix-config-options branch April 28, 2022 19:56
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