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

docs: update documentation for additionalRemoteWrite #2549

Merged
merged 1 commit into from Oct 12, 2022

Conversation

rnishtala-sumo
Copy link
Contributor

@rnishtala-sumo rnishtala-sumo commented Oct 3, 2022

Description

Update documentation around additionalRemoteWrite for kube-prometheus-stack

@rnishtala-sumo rnishtala-sumo requested a review from a team as a code owner October 3, 2022 22:00
@github-actions github-actions bot added the documentation documentation label Oct 3, 2022
@rnishtala-sumo rnishtala-sumo marked this pull request as draft October 3, 2022 22:14
@@ -36,7 +36,7 @@ kube-prometheus-stack: # For values.yaml
# ...
prometheusSpec:
# ...
remoteWrite:
additionalRemoteWrite:
# ...
Copy link
Contributor

Choose a reason for hiding this comment

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

no need for # ... it was indication that there was something to copy

also, we should search documentation for all remoteWrite mentions, as I'm not sure but we probably mention in multiple places that you need to copy it from our values.yaml which is no longer true

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok got it, will look for other references to remoteWrite. Also, trying to send some metrics from the k8s-collector to a sumo instance. Will try adding additional prometheus metrics.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

get this error when I try to send Prometheus metrics to Sumo (long) via Fluentd.
config error file="/fluentd/etc/fluent.conf" error_class=Fluent::ConfigError error="Invalid SumoLogic endpoint url: "

it's not clear where in endpoint url gets configured in fluentd, configured the following in values.yaml

Sumo API endpoint; Leave blank for automatic endpoint discovery and redirection

endpoint: "https://long-api......."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also @sumo-drosiek there doesn't seem to be any other mention of copying remoteWrite from values.yaml in the documentation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was able to send additional Prometheus metrics from a new metrics source. Also, removed copy references from the readme and added a note like below

NOTE: It is best practice to add custom configuration to a user-supplied values file and then use it like so (helm install -f my_values.yaml -n sumologic)

Copy link
Contributor

@sumo-drosiek sumo-drosiek left a comment

Choose a reason for hiding this comment

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

I would like to ask you also to go through the process of adding additional prometheus metrics, so we can have additional input on that, and make it less complicated (/more understable) 😓

@rnishtala-sumo rnishtala-sumo requested a review from a team October 4, 2022 14:26
@rnishtala-sumo rnishtala-sumo self-assigned this Oct 4, 2022
@rnishtala-sumo rnishtala-sumo marked this pull request as ready for review October 6, 2022 12:37
@rnishtala-sumo rnishtala-sumo force-pushed the rnishtala-doc-changes branch 2 times, most recently from 8639286 to 32e585a Compare October 6, 2022 21:07
@@ -271,15 +268,13 @@ To send custom metrics to Sumo Logic you need to update it to include a rule to
Make sure you include the same tag you created in your Fluentd configmap in the previous step.
Here is an example addition to the configuration file that will forward metrics to Sumo:

NOTE: Please add the below to a user-supplied values file (```helm install -f```)
Copy link
Contributor

Choose a reason for hiding this comment

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

incomplete command

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok makes sense, I could keep the one NOTE at the beginning and remove the rest of them. Would that work?

@sumo-drosiek
Copy link
Contributor

Generally I'm not sure about copying the same note in multiple places in one document
Also for all our docs Note's are usually bold

cc: @astencel-sumo

Copy link
Contributor

@sumo-drosiek sumo-drosiek left a comment

Choose a reason for hiding this comment

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

Thanks!

@swiatekm-sumo
Copy link
Contributor

@rnishtala-sumo the CI should be fine if you rebase on main.

Adding a note about user-supplied yaml configuration

Removed repeating note comments
@rnishtala-sumo
Copy link
Contributor Author

Fixes issue: #1822

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants