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

MGDSTRM-10557: Expose strimzi oauth metrics prometheus in order to assist investigating a slow authentication issue #910

Merged
merged 2 commits into from
Apr 19, 2023

Conversation

k-wall
Copy link
Contributor

@k-wall k-wall commented Apr 18, 2023

I also needed to apply the override mechanism to the kafka container in order to set kafka container environment variables, so I can actually turn on strimzi oauth metrics. The work for this part is a separate commit.

 env:
      - name: OAUTH_ENABLE_METRICS
        value: "true"

Here are screenshots showing the new metrics, demonstrating that the end to end is working.

Screenshot 2023-04-18 at 12 02 29

Screenshot 2023-04-18 at 12 09 23

@github-actions github-actions bot added the operator changes related to operator label Apr 18, 2023
@@ -554,6 +562,15 @@ private KafkaClusterTemplate buildKafkaTemplate(ManagedKafka managedKafka, int r
return templateBuilder.build();
}

private List<ContainerEnvVar> buildKafkaContainerEnvVars(ManagedKafka managedKafka) {
var kafkaOverride = this.overrideManager.getKafkaOverride(managedKafka.getSpec().getVersions().getStrimzi());
Copy link
Contributor Author

@k-wall k-wall Apr 18, 2023

Choose a reason for hiding this comment

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

I tried approaches introducing an env var type parameter to OperandOverride<E>) etc but the resulting code applyEnvironmentTo in less obvious that this list conversion in the method.

@sonarcloud
Copy link

sonarcloud bot commented Apr 18, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@k-wall k-wall requested review from grdryn and MikeEdgar April 18, 2023 11:11
@k-wall k-wall added this to the 0.36.0 milestone Apr 18, 2023
Copy link
Member

@grdryn grdryn left a comment

Choose a reason for hiding this comment

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

One small comment/question, but looks good to me.

Comment on lines +309 to +338
- pattern: "strimzi.oauth<type=(.+), context=(.+), kind=(.+), host=\"(.+)\", path=\"(.+)\", (.+)=(.+), (.+)=(.+), (.+)=(.+)><>(count|totalTimeMs):"
name: "strimzi_oauth_$1_$12"
type: COUNTER
labels:
context: "$2"
kind: "$3"
host: "$4"
path: "$5"
"$6": "$7"
"$8": "$9"
"$10": "$11"
- pattern: "strimzi.oauth<type=(.+), context=(.+), kind=(.+), host=\"(.+)\", path=\"(.+)\", (.+)=(.+), (.+)=(.+)><>(count|totalTimeMs):"
name: "strimzi_oauth_$1_$10"
type: COUNTER
labels:
context: "$2"
kind: "$3"
host: "$4"
path: "$5"
"$6": "$7"
"$8": "$9"
- pattern: "strimzi.oauth<type=(.+), context=(.+), kind=(.+), host=\"(.+)\", path=\"(.+)\", (.+)=(.+)><>(count|totalTimeMs):"
name: "strimzi_oauth_$1_$8"
type: COUNTER
labels:
context: "$2"
kind: "$3"
host: "$4"
path: "$5"
"$6": "$7"
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if these three entries are redundant? The 3 patterns below look like more generalised forms that would also cover these cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, they won't be redundant. They are matching different forms. The number of groups is different in each case.

Copy link
Member

@grdryn grdryn Apr 19, 2023

Choose a reason for hiding this comment

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

I had counted the number of groups in each case, and they appeared to be the same is what I meant. 🙂 It seems like the actual difference is that the above 3 entries are counters, while the below 3 are gauges. Here's a diff tool view with the above on the left, and the below on the right. Lines with differences are those in blue, with the actual differences on those lines highlighted in a slightly lighter blue:

image

Copy link
Contributor

@MikeEdgar MikeEdgar left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@k-wall k-wall merged commit d2122a6 into bf2fc6cc711aee1a0c2a:main Apr 19, 2023
@k-wall k-wall deleted the MGDSTRM-10557 branch April 19, 2023 07:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
operator changes related to operator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants