-
Notifications
You must be signed in to change notification settings - Fork 6
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 Sample App] Adding features LabelCount, DatapointCount, Norm Dist, and K8s cluster config #11
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Paurush - pl add more inline comments in the code to have a high-level overview of the functionality. Also - would be good to have a changelog of key changes you've made - could be a list of commits / PRs.
README.md
Outdated
- Docker | ||
- Docker Image Prometheus-Sample-App | ||
- Minikube |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Minikube | |
- A Kubernetes cluster |
This should work on any k8s cluster, not just minikube.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Aneurysm9 thanks. I have updated.
README.md
Outdated
- Minikube | ||
|
||
### Deployment on Kubernetes: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
### Deployment on Kubernetes: | |
### Deployment on Minikube: |
EKS is Kubernetes, too. This has some detail specific to minikube, so call that out here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Aneurysm9 thanks. I have updated.
README.md
Outdated
- Start Minikube dashboard | ||
```bash | ||
$ minikube dashboard |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the dashboard need to be running?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Aneurysm9 thanks. no its not a requirement. So I have removed the step.
README.md
Outdated
``` | ||
|
||
Currently, OTEL Collector is configured with Logging exporter. Since all 5 replica Prometheus-Sample-App pods will produce identical metric names and labels, so Prometheus Exporter should be configured with additional details to ensure distinct metrics identity. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any information we can include here regarding what sort of additional details need to be included and how to include them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Aneurysm9 I am unsure right now what can be done, my understanding is to generate unique source labels through configuration. But I am not entirely sure. So I have updated the comment.
metrics/metrics_collector.go
Outdated
var ( | ||
normDomain = flag.Float64("normal.domain", 0.0002, "The domain for the normal distribution.") | ||
normMean = flag.Float64("normal.mean", 0.00001, "The mean for the normal distribution.") | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should probably go in metrics_cli.go
. They're CLI flags so the seem a better fit there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Aneurysm9 thanks. I have updated.
metrics/metrics_collector.go
Outdated
lowerBound := math.Mod(rand.Float64(), 1) | ||
increment := math.Mod(rand.Float64(), 0.05) | ||
labels := datapointLabels(i, mc.labelKeys, mc.labelValues) | ||
for j := lowerBound; j < 1; j += increment { | ||
mc.histograms[idx].With(labels).Observe(j) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this also use a random distribution rather than incremental observations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Aneurysm9 thanks. I have updated.
metrics/metrics_collector.go
Outdated
Namespace: namespace, | ||
Name: fmt.Sprintf("summary%v", idx), | ||
Help: "This is my summary", | ||
Objectives: map[float64]float64{0.5: 0.05, 0.9: 0.01, 0.99: 0.001}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this change will do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have reversed this change. I was trying different quantile ranges, so left them there. I have reversed it now.
otel-collector-k8s-deployment.yaml
Outdated
- name: otelcol | ||
args: | ||
- --config=/conf/collector.yaml | ||
image: otel/opentelemetry-collector:0.18.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is ancient, isn't it? Can this be updated to a newer collector image?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Aneurysm9 thanks. I have updated.
Thanks. I have updated the comments. I included the list of changes in the issue(#10) and PR description. I couldnt find a changelog file in this project. |
Added following features:
Issue: 10
cc @alolita @Aneurysm9