Skip to content
This repository was archived by the owner on Oct 25, 2023. It is now read-only.

Updates to integrate CCloud Metrics SDK and Added Logging #38

Closed
wants to merge 2 commits into from

Conversation

nerdynick
Copy link

This removes the CCloudExporter from containing all logic to directly call the API and instead pass it through the Metrics SDK. The SDK adds on to the foundations originally created here, but extends functionality for auto discovery of topics for metrics, wildcard/All auto looked up for all topics on a metric, extended go routine usage with worker pools and worker caps, connection pooling, enhanced value checks, and more.

@nerdynick nerdynick changed the title <WIP> Updates to integrate CCloud Metrics SDK Updates to integrate CCloud Metrics SDK and Added Logging Jul 20, 2020
@Dabz
Copy link
Owner

Dabz commented Jul 20, 2020

I will review the PR later on, but I am not really keen on having an external dependency on the Metrics API TBH 😅

Historically, the Metrics API moved quite a lot and I had to do a few "emergencies" patch as some updates broke previous versions of the exporter. As well, the methods in Query.go and Descriptors.go had to be updated to handle properly new labels or metrics and are the results of quite a few internal discussions.

For the moment, I think we should keep the query logic inside ccloudexporter, it would just be easier to maintain. We could migrate to the Go SDK later on.

As well, this PR include many changes, it would be nice to break the PR into smaller one.

@nerdynick
Copy link
Author

The SDK approach was done as to centralize all changes that come with the metrics API. So that not only does the CCloudExporter get the changes and fixes, other things like the CLI can. This makes it so that fixes and improvements can come from more directions rather then just the CCloudExporter.

As well as I was also have issues with how tightly coupled the Prometheus Logic was with the query logic making it harder to make the original work efforts possible. This new path separates the realms completely, allowing for more complex API calls or changes to happen without needing to test or modify the prometheus workflows within CCloudExporter.

Some of the other changes involved are in relation to issues/bug I found while trying to test as well as the additions of logging to assist in the testing/development efforts.

@Dabz
Copy link
Owner

Dabz commented Jul 20, 2020

The SDK approach was done as to centralize all changes that come with the metrics API. So that not only does the CCloudExporter get the changes and fixes, other things like the CLI can. This makes it so that fixes and improvements can come from more directions rather then just the CCloudExporter.

This is quite legit, and I think that having some kind of SDK would be great. Nonetheless, this is an important change, much more than just a "fix" or an improvement, it changes the architecture and the responsibility of the source code. The current source code and the current behavior might look simple, but it's actually the conclusion of a lot of thinking with the Metrics API team.

wildcard/All auto looked up for all topics on a metric

For example, it used to be the default behavior of the ccloudexporter, yet we changed it following recommendations from the Metrics API team (#32 and #31). This is not a limitation, but protection for both the Metrics API and the user of the exporter.

If there is an SDK that becomes popular, is well maintained, and fits our need, ccloudexporter might switch to it, but I do think that it is something that needs to be carefully planned, discussed, and tested.

@nerdynick nerdynick closed this Oct 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants