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

Prometheus set datastream name #5336

Merged
merged 9 commits into from Feb 24, 2023
Merged

Prometheus set datastream name #5336

merged 9 commits into from Feb 24, 2023

Conversation

gizas
Copy link
Contributor

@gizas gizas commented Feb 20, 2023

  • Enhancement

What does this PR do?

This PR adds the data_stream.dataset textbox for the Prometheus Integration package in UI, in order users to be able to change the dataset name for all the supported Prometheus configurations (Collector, PromQL, Remote_write)
Tested with latest 8.6.2 version of ES

Checklist

  • I have reviewed tips for building integrations and this pull request is aligned with them.
  • I have verified that all data streams collect metrics.
  • I have added an entry to my package's changelog.yml file.
  • I have verified that Kibana version constraints are current according to guidelines.

How to test this PR locally

  1. Clone this PR locally
  2. Change to Prometheus package folder cd /elastic/integrations/packages/prometheus
  3. Run elastic-package build
  4. Create Elastic Stack: elastic-package stack up -d -vvv --version=8.6.2
  5. Provide different configurations for Prometheus by following guide: https://github.com/elastic/observability-dev/blob/main/docs/infraobs/cloudnative-monitoring/testing/prometheus-integration-on-k8s.md

Related issues

Screenshots

Default Collector config:
collector_default

Collector Configuration changed:
collector_changed

Collector with new dataset name:
Screenshot 2023-02-20 at 4 46 50 PM

PromQL Default:
query_default

query_

Changed Promql dataset name:
query_results

Remote Write default:
Screenshot 2023-02-20 at 5 07 15 PM

Changing Default Configuration for remote_write:

remote_write

pasole_remote_write

@gizas gizas requested a review from a team as a code owner February 20, 2023 15:22
@gizas gizas added the Team:Cloudnative-Monitoring Label for the Cloud Native Monitoring team label Feb 20, 2023
@elasticmachine
Copy link

elasticmachine commented Feb 20, 2023

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2023-02-24T12:07:10.655+0000

  • Duration: 16 min 35 sec

Test stats 🧪

Test Results
Failed 0
Passed 9
Skipped 0
Total 9

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

@elasticmachine
Copy link

elasticmachine commented Feb 20, 2023

🌐 Coverage report

Name Metrics % (covered/total) Diff
Packages 100.0% (0/0) 💚
Files 100.0% (0/0) 💚
Classes 100.0% (0/0) 💚
Methods 88.889% (8/9) 👎 -7.013
Lines 100.0% (0/0) 💚 11.181
Conditionals 100.0% (0/0) 💚

@@ -41,3 +41,7 @@ condition: ${kubernetes_leaderelection.leader} == true
{{#if timeout}}
{{timeout}}
{{/if}}
{{#if data_stream.dataset}}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we do not need this if, in case of providing a default value

the same for other data_streams

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, removed with fb2f0ef

@ChrsMark
Copy link
Member

@gizas could you provide more details on why we need this addition?
I would expect that this information should be added automatically by Agent, isn't that the case?

@gizas
Copy link
Contributor Author

gizas commented Feb 21, 2023

@ChrsMark the agent indeed provides a default value of dataset per datastream. This PR adds the flexibility to the user to direct certain metrics to certain datastream (via the dataset naming).

Especially for collector scenario, imagine that user can scrape two diffrent exporters and separate metrics would had been saved in the same prometheus.collector in the past. Now it can create two policies and save them to different ones

@ChrsMark
Copy link
Member

Thanks @gizas ! That make sense as an advanced option.
What is concerning to me here is that the specific change implies that the Prometheus package stands as an "input" package too, right? This distinction is not clear yet for this package and it feels like we mix 2 different things here. Are there any plans or discussions regarding if there will be a generic Prometheus input package?
This specific change in this PR would fit to an input package but not to a Prometheus specific one (a package that collects Prometheus server metrics).

@gizas
Copy link
Contributor Author

gizas commented Feb 21, 2023

I would not say that Prometheus Package stands as an "input." solely. In our team we still promote remote_write and we underline that this Integration aims for the Prometheus Server scraping.

But correctly this change is towards the input. We integrated here a "feature" of the input. FYI we tried to support an ask from the SREs of the MKI project to be able to change the dataset name.

Additionally we have in our backlog the Prometheus Input story and we have made some discussions on what differences this should have comparing to this integration. But until the Prometheus Inout package is out there, we added this change here

multi: false
default: prometheus.collector
required: true
show_user: true
Copy link
Contributor

Choose a reason for hiding this comment

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

@gizas is it intentional to make this setting not a advanced option?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. What do you think?
I had changed my mind and thought it can be better like that. Dont have a strong opinion

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking about this setting more as an "advanced option" to not overwhelm the configuration view, but it is not a strong opinion as well

@gizas gizas merged commit 47c5df2 into main Feb 24, 2023
@gizas gizas deleted the prometheus_set_datastream branch February 24, 2023 12:24
@elasticmachine
Copy link

Package prometheus - 1.3.0 containing this change is available at https://epr.elastic.co/search?package=prometheus

agithomas pushed a commit to agithomas/integrations that referenced this pull request Mar 20, 2023
* Updating Prometheus Integration to support Datastream.daatset Name settting
agithomas pushed a commit to agithomas/integrations that referenced this pull request Mar 21, 2023
* Updating Prometheus Integration to support Datastream.daatset Name settting
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team:Cloudnative-Monitoring Label for the Cloud Native Monitoring team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adding support for dataset configuration for Prometheus Integration
6 participants