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

Add more attributes extracted for traces #438

Merged
merged 10 commits into from Mar 3, 2020
Merged

Conversation

pmm-sumo
Copy link
Contributor

@pmm-sumo pmm-sumo commented Feb 21, 2020

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
  • ci/build.sh
  • Redeploy fluentd and fluentd-events pods
  • Confirm events, logs, and metrics are coming in

@pmm-sumo
Copy link
Contributor Author

@sumo-drosiek how about using the new image with our Collector distribution?
@perk-sumo could you take a look?

@sumo-drosiek
Copy link
Contributor

name: "omnition/opentelemetry-collector-contrib"
tag: "0.0.5"
name: "sumologic/opentelemetry-collector"
tag: "0.0.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

@pmaciolek could you update?

Copy link
Contributor

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.

@perk-sumo
Copy link
Contributor

Copy link
Contributor

@perk-sumo perk-sumo left a 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 👍

@sumo-drosiek
Copy link
Contributor

@perk-sumo Is it up to date? https://hub.docker.com/r/sumologic/opentelemetry-collector/tags

Yes, this is the registry we're using and that's where CI is putting the images.
I can see there is the v0.2.6.1 release in there:
https://github.com/SumoLogic/opentelemetry-collector-contrib/releases/tag/v0.2.6.1
https://hub.docker.com/layers/sumologic/opentelemetry-collector/0.2.6.1/images/sha256-21cb9533bbd3b65ed3f4258516bd254ac7c221374635d547d71b99c98c473e66?context=explore

I asked before I realized that it's updating after tagging

@sumo-drosiek
Copy link
Contributor

sumo-drosiek commented Feb 28, 2020

I think we are good to go. @pmm-sumo could you review my changes?

Copy link
Contributor Author

@pmm-sumo pmm-sumo left a 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 }}
Copy link
Contributor Author

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

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?

Copy link
Contributor

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

bb9c9b0

Copy link
Contributor

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.

Copy link
Contributor

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

@pmm-sumo pmm-sumo changed the title [WIP] Add more attributes extracted for traces Add more attributes extracted for traces Feb 28, 2020
Copy link
Contributor

@perk-sumo perk-sumo left a 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.

Copy link
Contributor

@perk-sumo perk-sumo left a 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.

@sumo-drosiek sumo-drosiek merged commit 46af55b into master Mar 3, 2020
@sumo-drosiek sumo-drosiek deleted the otelcol-ext-config branch March 3, 2020 12:27
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.

None yet

4 participants