Skip to content

Conversation

nlamirault
Copy link
Contributor

@nlamirault nlamirault commented Nov 28, 2022

Pull Request

Related issue

What does this PR do?

  • Creates a ServiceMonitor Kubernetes resources.

Prometheus metrics must be enabled using the --experimental-enable-metrics CLI argument

PR checklist

Please check if your PR fulfills the following requirements:

  • Does this PR fix an existing issue, or have you listed the changes applied in the PR description (and why they are needed)?
  • Have you read the contributing guidelines?
  • Have you made sure that the title is accurate and descriptive of the changes?

Thank you so much for contributing to Meilisearch!

Signed-off-by: Nicolas Lamirault <[email protected]>
Signed-off-by: Nicolas Lamirault <[email protected]>
Signed-off-by: Nicolas Lamirault <[email protected]>
Copy link
Contributor

@alallema alallema left a comment

Choose a reason for hiding this comment

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

Hi @nlamirault,
Thank you this PR! 🔥
Could you add your new options in the parameters list in the template README? And also could you fill in the description for your PR?
Thank again for contributing to Meilisearch ❤️

@brunoocasali
Copy link
Member

brunoocasali commented Feb 23, 2023

@alallema I think this PR will be necessary to the adoption of the new experimental feature meilisearch/meilisearch#3523

So what I suggest you to do is:

  • Fix parameters list like you suggested to the contributor
  • Add comments and point to the engine issue regarding this experimental feature.
  • Add support to Authorization token when a master key protects the instance.

Signed-off-by: Nicolas Lamirault <[email protected]>
Signed-off-by: Nicolas Lamirault <[email protected]>
Signed-off-by: Nicolas Lamirault <[email protected]>
Signed-off-by: Nicolas Lamirault <[email protected]>
Signed-off-by: Nicolas Lamirault <[email protected]>
Signed-off-by: Nicolas Lamirault <[email protected]>
@alallema
Copy link
Contributor

Hi @nlamirault,
Thank you so much for continuing to update your code each time ❤️ . I didn't forget you. I will finish by merging your PR, I just need to check some stuff before and I still don't have time to do it, but I will.
Thank you so much for your patience 😊

@alallema
Copy link
Contributor

alallema commented Jun 7, 2023

Hi @nlamirault,
Sorry to come back to this PR so late. I finally checked it.
Rather than using the CLI command --experimental-enable-metrics, would it be possible to add the MEILI_EXPERIMENTAL_ENABLE_METRICS: "false" parameter to the environment variables and then if {{- if.Values.serviceMonitor.enabled -}} set it as (eq.Values.environment.MEILI_EXPERIMENTAL_ENABLE_METRICS "true")
I'm not an expert, so I'd like your opinion. I can implement it in a subsequent PR if it's annoying.
Thank you 😊

matchLabels:
{{- include "meilisearch.selectorLabels" . | nindent 6 }}
endpoints:
- port: http
Copy link
Contributor

Choose a reason for hiding this comment

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

Haven't tested this myself yet, but I was looking at adding a ServiceMonitor to my deployment, and I think you'd need something like this to support authentication, right?

  endpoints:
    - bearerTokenSecret:
        name: meilisearch-master-key
        key: MEILI_MASTER_KEY
...

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @mike-stewart,
You're absolutely right thanks! I'll add it to the PR as well as the flag for activated analytics.

@alallema alallema requested a review from brunoocasali June 27, 2023 15:23
@alallema
Copy link
Contributor

bors merge

@meili-bors
Copy link
Contributor

meili-bors bot commented Jun 27, 2023

Build succeeded:

@meili-bors meili-bors bot merged commit 3926596 into meilisearch:main Jun 27, 2023
@meili-bot
Copy link
Contributor

This message is sent automatically

Thank you very much for submitting your PR! Did you know that throughout the month of June we’re holding a rafle?
If you share the link to your merged PR in our #giveaway Discord channel, you’ll automatically join a lottery for a chance at winning some Meiliswag. 🙂
Don’t hesitate to join us: https://discord.com/channels/1006923006964154428/1111273670657200198

@mike-stewart mike-stewart mentioned this pull request Jun 30, 2023
3 tasks
meili-bors bot added a commit that referenced this pull request Jun 30, 2023
186: Fix Bug in ServiceMonitor r=brunoocasali a=mike-stewart

# Pull Request

## What does this PR do?
It appears there might be a bug in #153, as there should not be a dash in front of bearerTokenSecret. This is adding it as a second endpoint, when in fact it should be setting the bearerTokenSecret on the http endpoint.

## PR checklist
Please check if your PR fulfills the following requirements:
- [x] Does this PR fix an existing issue, or have you listed the changes applied in the PR description (and why they are needed)?
- [x] Have you read the contributing guidelines?
- [x] Have you made sure that the title is accurate and descriptive of the changes?

Thank you so much for contributing to Meilisearch!


Co-authored-by: Mike Stewart <[email protected]>
meili-bors bot added a commit that referenced this pull request Jun 30, 2023
186: Fix Bug in ServiceMonitor r=brunoocasali a=mike-stewart

# Pull Request

## What does this PR do?
It appears there might be a bug in #153, as there should not be a dash in front of bearerTokenSecret. This is adding it as a second endpoint, when in fact it should be setting the bearerTokenSecret on the http endpoint.

## PR checklist
Please check if your PR fulfills the following requirements:
- [x] Does this PR fix an existing issue, or have you listed the changes applied in the PR description (and why they are needed)?
- [x] Have you read the contributing guidelines?
- [x] Have you made sure that the title is accurate and descriptive of the changes?

Thank you so much for contributing to Meilisearch!


Co-authored-by: Mike Stewart <[email protected]>
Co-authored-by: Bruno Casali <[email protected]>
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.

5 participants