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

MSSQL Integration: Adds query_config_path to allow for custom metrics through custom exporter config file #5768

Merged
merged 4 commits into from
Nov 27, 2023
Merged

MSSQL Integration: Adds query_config_path to allow for custom metrics through custom exporter config file #5768

merged 4 commits into from
Nov 27, 2023

Conversation

StefanKurek
Copy link
Contributor

@StefanKurek StefanKurek commented Nov 14, 2023

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

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

@mattdurham mattdurham self-assigned this Nov 14, 2023
@@ -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"`
Copy link
Collaborator

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")
Copy link
Collaborator

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.

Copy link
Collaborator

@mattdurham mattdurham left a 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.

Copy link
Contributor

@clayton-cornell clayton-cornell left a comment

Choose a reason for hiding this comment

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

Some doc suggestions

@clayton-cornell clayton-cornell added the type/docs Docs Squad label across all Grafana Labs repos label Nov 16, 2023
@StefanKurek
Copy link
Contributor Author

@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!

Copy link
Contributor

@clayton-cornell clayton-cornell left a 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

Copy link
Collaborator

@mattdurham mattdurham left a comment

Choose a reason for hiding this comment

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

LGTM

@mattdurham
Copy link
Collaborator

Getting a lint error.

@StefanKurek
Copy link
Contributor Author

@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.

@mattdurham
Copy link
Collaborator

Approved it to run.

@clayton-cornell
Copy link
Contributor

@StefanKurek @mattdurham It's passing the build now. Since it's reviewed and approved, I'll merge it into Main. :-)

@clayton-cornell clayton-cornell merged commit 7f8bb76 into grafana:main Nov 27, 2023
10 checks passed
@StefanKurek StefanKurek deleted the feat/mssql_integration_custom_metrics branch November 28, 2023 00:16
BarunKGP pushed a commit to BarunKGP/grafana-agent that referenced this pull request Feb 20, 2024
… 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
@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. type/docs Docs Squad label across all Grafana Labs repos
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants