-
Notifications
You must be signed in to change notification settings - Fork 488
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
MSSQL Integration: Adds query_config_path to allow for custom metrics through custom exporter config file #5768
MSSQL Integration: Adds query_config_path to allow for custom metrics through custom exporter config file #5768
Conversation
@@ -40,6 +41,7 @@ type Arguments struct { | |||
MaxIdleConnections int `river:"max_idle_connections,attr,optional"` | |||
MaxOpenConnections int `river:"max_open_connections,attr,optional"` | |||
Timeout time.Duration `river:"timeout,attr,optional"` | |||
QueryConfigPath string `river:"query_config_path,attr,optional"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The more river centric path for this would for this to be query_config and allow them to use local.file to get the content. That way they could get it in a myriad number of ways or inline if they really really wanted to.
if errors.Is(err, os.ErrNotExist) { | ||
return errors.New("query_config_path must be a valid path of a YAML config file") | ||
} else { | ||
return errors.New("query_config_path file has issues") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should wrap or fmt.Error the existing error from stat.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
River centric changes and error handling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some doc suggestions
docs/sources/flow/reference/components/prometheus.exporter.mssql.md
Outdated
Show resolved
Hide resolved
docs/sources/flow/reference/components/prometheus.exporter.mssql.md
Outdated
Show resolved
Hide resolved
docs/sources/flow/reference/components/prometheus.exporter.mssql.md
Outdated
Show resolved
Hide resolved
@mattdurham @clayton-cornell Hello. I've looked at your feedback and made changes. I ended up going further and makign both a query_config_file and query_config to closer match the patterns of SNMP and Blackbox. Let me know what you think. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small tweak to the docs
docs/sources/flow/reference/components/prometheus.exporter.mssql.md
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Getting a lint error. |
@clayton-cornell could you run the last test? It is hard to ensure locally, as I seem to get a bunch of linter errors outside of the integration under change that don't appear to show up in the CI. |
…cs through custom exporter config file
Approved it to run. |
@StefanKurek @mattdurham It's passing the build now. Since it's reviewed and approved, I'll merge it into Main. :-) |
… through custom exporter config file (grafana#5768) * Adds query_config_path to mssql integration to allow for custom metrics through custom exporter config file * Updates query_config_path to agent flow and fixes tests/docs * Adds both query_config and query_config_file to mssql integration * Removes query_config_file from mssql config params
PR Description
MSSQL Integration: Adds query_config_path to allow for custom metrics through custom exporter config file.
Updates both integration and relevant documentation.
Which issue(s) this PR fixes
NA
Notes to the Reviewer
PR Checklist