-
Notifications
You must be signed in to change notification settings - Fork 183
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
Add more attributes extracted for traces #438
Conversation
@sumo-drosiek how about using the new image with our Collector distribution? |
@perk-sumo Is it up to date? https://hub.docker.com/r/sumologic/opentelemetry-collector/tags |
deploy/helm/sumologic/values.yaml
Outdated
name: "omnition/opentelemetry-collector-contrib" | ||
tag: "0.0.5" | ||
name: "sumologic/opentelemetry-collector" | ||
tag: "0.0.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.
@pmaciolek could you update?
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.
That's really good point - please use the tagged otelcol release.
Yes, this is the registry we're using and that's where CI is putting the images. |
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.
Except for the otelcol image version LGTM 👍
I asked before I realized that it's updating after tagging |
I think we are good to go. @pmm-sumo could you review my changes? |
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 am a bit concerned of flexibility with configuring any processors/attributes/etc. Perhaps there's no clear solution but we should research if there's something available
send_batch_size: 1024 | ||
timeout: 5s | ||
send_batch_size: {{ .Values.otelcol.processors.batch.send_batch_size }} | ||
timeout: {{ .Values.otelcol.processors.batch.timeout }} |
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.
@perk-sumo The challenge here is that there is a number of processors: https://github.com/open-telemetry/opentelemetry-collector/tree/master/processor - the list is actually growing and their properties might change over time.
An ideal solution would to provide a template with the full config file, but we are not sure how to tackle it. Do you have some suggestions?
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.
@sumo-drosiek perhaps we should minimally add sampling processors: https://github.com/open-telemetry/opentelemetry-collector/tree/master/processor#sampling-processor ?
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.
As I think about it more and more, we should just take full config branch and put it as a configmap. If user have own processors it's gonna add changes to values or config file anyway. Additionally we want to support --set
flags
@perk-sumo any thoughts?
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.
not so nice proposal, but had to support dynamic value for url :D
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'd like to keep some defaults for clients who don't want to deal with the config that much.
We could do it like fluent-bit: provide sane defaults in config.yaml file with some flags in values.yaml and if someone want to have a totally different config provide a special key in values.yaml for that.
We could also provide the template config but I'm not sure if that should go to values.yaml or we should create a new file for that like values.otelcol.yaml or sth. With that client would need to copy and paste something into values.yaml by hand.
I don't like the idea of whole otelcol config inside default values.yaml because it will grow really huge and that doesn't seem right.
Please take a look at:
https://github.com/helm/charts/blob/master/stable/fluent-bit/values.yaml#L146
https://github.com/helm/charts/blob/master/stable/fluent-bit/templates/config.yaml#L1
It's similar to what @sumo-drosiek did.
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.
We could do it like fluent-bit
I think that our case differs. We are able to use yaml
. fluentbit were obligated to convert yaml configuration into fluent-bit specific format.
I would like to stick to the values.yaml
. It's more clear and in both cases values.yaml
will be growing
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.
Let's drop the config file and go with configmap instead.
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'm good with the change - let's put the config into values yaml, iterate quickly and try to refactor this when we have better understanding of which values should be extracted.
Description
Adds more fields extracted, when used with our extended OpenTelemetry Collector build. Needs adding the image first (@mrumian-sumo, can you help?)
Testing performed