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

Initial configuration files for 3 main applications and exporters build script #6

Conversation

Nicolas-MacBeth
Copy link
Contributor

@Nicolas-MacBeth Nicolas-MacBeth commented Jul 22, 2020

OpenTelemetry Collector config for JVM, MySQL, Apache, DogStatsD, host and custom metrics collection.

Update: Waiting on collector-contrib version upgrade to see if it fixes build error before merging.

@google-cla google-cla bot added the cla: yes label Jul 30, 2020
Copy link
Contributor

@james-bebbington james-bebbington left a comment

Choose a reason for hiding this comment

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

Just some minor comments

.build/tar/generate_tar.sh Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
@Nicolas-MacBeth Nicolas-MacBeth marked this pull request as ready for review August 5, 2020 21:39
@Nicolas-MacBeth Nicolas-MacBeth changed the title Draft: Initial configuration files for 3 main applications Initial configuration files for 3 main applications and exporters build script Aug 6, 2020
Copy link
Contributor

@james-bebbington james-bebbington left a comment

Choose a reason for hiding this comment

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

I finally merged my large PR so you will also need to rebase.

.build/tar/build_exporters.sh Outdated Show resolved Hide resolved
.build/tar/build_exporters.sh Outdated Show resolved Hide resolved
.build/tar/build_exporters.sh Outdated Show resolved Hide resolved
.build/tar/build_exporters.sh Outdated Show resolved Hide resolved
.build/tar/build_exporters.sh Show resolved Hide resolved
config/config-mysql_apache_jvm.yaml Outdated Show resolved Hide resolved
config/config-mysql_apache_jvm.yaml Outdated Show resolved Hide resolved
config/config-mysql_apache_jvm.yaml Outdated Show resolved Hide resolved
config/config-mysql_apache_jvm.yaml Outdated Show resolved Hide resolved
config/config-mysql_apache_jvm.yaml Outdated Show resolved Hide resolved
.build/tar/build_exporters.sh Outdated Show resolved Hide resolved
config/config-mysql_apache_jvm.yaml Outdated Show resolved Hide resolved
Copy link
Contributor

@james-bebbington james-bebbington left a comment

Choose a reason for hiding this comment

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

See question about metric prefix. I'm actually a little confused on the goal of this currently

config/config-mysql_apache_jvm.yaml Outdated Show resolved Hide resolved
config/config-mysql_apache_jvm.yaml Outdated Show resolved Hide resolved
config/config-mysql_apache_jvm.yaml Outdated Show resolved Hide resolved
config/config-mysql_apache_jvm.yaml Outdated Show resolved Hide resolved
config/config-mysql_apache_jvm.yaml Outdated Show resolved Hide resolved
exporters:
stackdriver:
project: # stackdriver will use the default GCE credentials
metric_prefix: # empty to prevent the custom.googleapis.com/ prefix
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't we need to add custom.googleapis.com? If we don't wouldn't the metrics get rejected?

What's the intention here btw? Any explicitly listed transformed metrics get sent to the corresponding agent metrics, and any other metrics are exported as custom metrics?

Copy link
Contributor Author

@Nicolas-MacBeth Nicolas-MacBeth Aug 14, 2020

Choose a reason for hiding this comment

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

The idea here is that this config "mimicks" the agent and actually exports metrics in the same format as it would for third-party applications. So all the metrics in this file need to be prefixed by agent.googleapis.com. The reason why I hadn't just put the prefix in the stackdriver exporter was to allow some metrics to go through without that prefix, but I know now that you need a certain prefix for cloud moniroting. This means that within a certain receiver's metrics (say Java), I would need to filter out the "agent" metrics and put them in a separate pipeline from the "normal" ones. However, after discussions with Quentin, I will just whitelist the metrics wanted and ignore the rest, which means I can simply put them through the same exporter (I'll make an additional exporter which won't be used in this config but can be used by a user for custom metrics), and it will reduce the cost of additional metrics the user may not want.

To recap I'll filter everything out but the metrics that are used to populate the pre-made dashboards on Cloud Monitoring which trigger automatically from the third-party metrics (JVM, MySQL and Apache)

Copy link
Contributor

@james-bebbington james-bebbington Aug 15, 2020

Choose a reason for hiding this comment

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

I think the intent to use the agent prefix for curated metrics and fall back to using the custom prefix for any other metrics is valid. To me it seems a bit limiting for users if they can't easily export any metrics that aren't on the list of curated agent metrics. I'm not sure that what you had in the config before was correct though, as I think you would need to explicitly add the custom prefix for any of the metrics that you don't have a transform set up - is that what happens when you set metric_prefix to blank? (I thought that would mean you don't set any prefix for non transformed metrics so they will end up not getting sent at all?)

But in terms of whether or not it is actually important to let users export non-curated (non-agent prefixed) metrics, I'm not sure - Quentin would be more of an expert than me, so if he recommends doing it this way for now, then this LGTM.

Copy link
Contributor Author

@Nicolas-MacBeth Nicolas-MacBeth Aug 15, 2020

Choose a reason for hiding this comment

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

Yeah I agree it's limiting, so what I'll do is a setup similar to the one I had before. I'll manually set those "curated" metrics to have the agent.googleapis.com, and it'll be up to the user to add their own prefix when renaming if they want more than the "curated" metrics. At least this way they're not stuck with the agent.googleapis.com prefix on all the metrics which would make cloud monitoring block the extra ones, if they're not those curated ones. Doesn't sound ideal either way, but at least this way the user CAN get those metrics, they just need to transform their name to include a prefix. It comes back to the same for the agent metrics but limits the users less for the additional metrics, but by default they are not collected. Do you agree?

Copy link
Contributor

@james-bebbington james-bebbington Aug 16, 2020

Choose a reason for hiding this comment

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

I would suggest:

  • For all agent metrics, set the agent.googleapis.com prefix specifically as part of the transform (as you suggested)
  • In the stackdriver exporter config, specify the custom.googleapis.com prefix, so that any metrics that haven't already been give the agent.googleapis.com prefix will use the custom prefix
  • Ensure that in the default config you are filtering out all metrics that are not explicitly transformed to agent metrics
  • Test this to make sure it works as expected

That way all the user needs to do if they want to include additional custom metrics is to change the filter (they can transform the additional metrics too if desired, but they shouldn't have to do that).

Copy link
Contributor Author

@Nicolas-MacBeth Nicolas-MacBeth Aug 16, 2020

Choose a reason for hiding this comment

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

Okay, sounds good! Could you check Gitter please, I sent you a message there! Thanks!

Copy link
Contributor

@james-bebbington james-bebbington Aug 17, 2020

Choose a reason for hiding this comment

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

Replied.

As for my comment above, it actually should work as I stated as per this recent change: census-ecosystem/opencensus-go-exporter-stackdriver#273

Doesn't look like that change was released yet though. I'm going to release this now and then create a PR to update contrib. After that, maybe we can try updating this config.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sent you another Gitter message (can't use Gchat sorry)

Copy link
Contributor

Choose a reason for hiding this comment

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

Contrib was released with this change I believe. You can try updating the version of Contrib and testing if this now works (may help resolve your other dependency issue as well)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated the config, but still getting the same build error. If you pull this branch, do you get the same thing?

config/config-mysql_apache_jvm_statsd.yaml Outdated Show resolved Hide resolved
config/config-mysql_apache_jvm_statsd.yaml Outdated Show resolved Hide resolved
config/config-mysql_apache_jvm_statsd.yaml Outdated Show resolved Hide resolved
config/config-mysql_apache_jvm_statsd.yaml Outdated Show resolved Hide resolved
config/config-mysql_apache_jvm_statsd.yaml Outdated Show resolved Hide resolved
config/config-mysql_apache_jvm_statsd.yaml Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@google-cla
Copy link

google-cla bot commented Aug 18, 2020

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added cla: no and removed cla: yes labels Aug 18, 2020
@Nicolas-MacBeth
Copy link
Contributor Author

@googlebot I fixed it.

@google-cla google-cla bot added cla: yes and removed cla: no labels Aug 18, 2020
Copy link
Contributor

@james-bebbington james-bebbington left a comment

Choose a reason for hiding this comment

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

CI failed with:

impi errors:
cmd/otelopscol/components.go: Import groups are not in the proper order: ["Third party" "Third party" "Local"] impi verification failed: Found 1 errors
Makefile:63: recipe for target 'impi' failed

Can you run make presubmit and fix any errors?

README.md Outdated Show resolved Hide resolved
Nicolas-MacBeth and others added 2 commits September 8, 2020 09:18
Co-authored-by: James Bebbington <jbebbington@google.com>
@james-bebbington james-bebbington merged commit 65ebf83 into GoogleCloudPlatform:master Sep 10, 2020
JonathanWamsley pushed a commit to observIQ/opentelemetry-operations-collector that referenced this pull request Jan 21, 2022
* Support fluent-bit windows event log plugin
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants