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

RFC: Observability API #97

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

R-Lawton
Copy link

@R-Lawton R-Lawton commented Jul 23, 2024

What are people's thoughts on the original suggestion of the observability CR? After a discussion i had with Jim, I think the observability policy is good but I would like others' opinions too.

@R-Lawton R-Lawton force-pushed the rfc-observability branch 3 times, most recently from 34cb471 to 3b91376 Compare July 25, 2024 13:42
@R-Lawton R-Lawton marked this pull request as ready for review July 25, 2024 13:42
@R-Lawton R-Lawton changed the title RFC: Observability Policy RFC: Observability API Jul 25, 2024
@R-Lawton R-Lawton force-pushed the rfc-observability branch from afbb0c4 to 2864c28 Compare July 25, 2024 13:59
rfcs/0000-observability-api.md Outdated Show resolved Hide resolved
rfcs/0000-observability-api.md Outdated Show resolved Hide resolved
rfcs/0000-observability-api.md Outdated Show resolved Hide resolved
rfcs/0000-observability-api.md Outdated Show resolved Hide resolved
rfcs/0000-observability-api.md Outdated Show resolved Hide resolved
rfcs/0000-observability-api.md Outdated Show resolved Hide resolved
@R-Lawton R-Lawton force-pushed the rfc-observability branch from 4b9fc27 to 19b1fdb Compare July 30, 2024 13:55
@maleck13
Copy link
Collaborator

maleck13 commented Jul 31, 2024

So discussed with @R-Lawton in person, but there seems to be some redundancy in the configuration and needs a user to understand the names and configuration options for underlying components. Also a separate CRD doesn't seem needed IMO if we condense the config

I think we can simplify this down somewhat into a two phase approach

  1. offer log and tracing options in the kuadrant CR (but they will only affect Authorino and Limitador) in phase 1 and be as simple as:
logging:
   level: debug
tracing:
  common tracing config applied to both limitador and authorino
metrics:
  enabled: false (removes service monitors) enabled by default

Where by default it is set to error level for logging and a default logging config plus , metrics defaulted to on.

We can then expand beyond this in a second phase

add comming configmap for expressing observability values that can be loaded as env vars (logging for example) so that it changes all components including the operators . Add in new options around alerts and dashboards as needed.

The status of the kuadrant resource can reflect what has been configured:

logging:
   authorino: debug
   limitador: debug
tracing:
  ... 

WDYT @Boomatang @R-Lawton

@R-Lawton
Copy link
Author

R-Lawton commented Aug 1, 2024

  1. offer log and tracing options in the kuadrant CR (but they will only affect Authorino and Limitador) in phase 1 and be as simple as:
logging:
   level: debug
tracing:
  common tracing config applied to both limitador and authorino
metrics:
  enabled: false (removes service monitors) enabled by default

Where by default it is set to error level for logging and a default logging config plus , metrics defaulted to on.

@maleck13

By default the log level is info, what are your thoughts about changing it to error?

@Boomatang
Copy link
Contributor

@maleck13

To me this makes sense, I do think we need the focus more on the action being preformed over the component being configured.

How I see this now is we should remove the current ability to configure Limitador from the kuadrant CR in favor of the more action based configuration. If you have not seen the discussion, #102 that suggest removing the work done for RFC0006 I will take your comment as the answer for that discussion.

After talking with @R-Lawton, we can not see any iteration of this RFC that would not require the removal of the past configuration implantation. So I have opened a PR to remove that work, Kuadrant/kuadrant-operator#795

Signed-off-by: R-Lawton <[email protected]>

Signed-off-by: R-Lawton <[email protected]>
@R-Lawton R-Lawton requested a review from KevFan August 19, 2024 13:57
Copy link
Contributor

@KevFan KevFan left a comment

Choose a reason for hiding this comment

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

Looks generally good, I've left some minor suggestions 👍

rfcs/0000-observability-api.md Outdated Show resolved Hide resolved
rfcs/0000-observability-api.md Outdated Show resolved Hide resolved
rfcs/0000-observability-api.md Outdated Show resolved Hide resolved
rfcs/0000-observability-api.md Outdated Show resolved Hide resolved
rfcs/0000-observability-api.md Outdated Show resolved Hide resolved
rfcs/0000-observability-api.md Outdated Show resolved Hide resolved
rfcs/0000-observability-api.md Outdated Show resolved Hide resolved
rfcs/0000-observability-api.md Outdated Show resolved Hide resolved
@alexsnaps alexsnaps added the RFC Request For Comments label Aug 20, 2024
…e location to the Kuadrant CR Signed-off-by: R-Lawton <[email protected]>

Signed-off-by: R-Lawton <[email protected]>
@R-Lawton R-Lawton requested a review from KevFan August 21, 2024 13:51
Copy link
Contributor

@KevFan KevFan left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍

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

Successfully merging this pull request may close these issues.

7 participants