Adding simple support for backend telemetry. #19257

dennisoelkers · 2024-05-06T07:01:16Z

Description

Motivation and Context

This PR is providing a simple foundation for sending telemetry from the backend. It bootstraps the client using the (already available) API key and endpoint and adds a simple periodical (TelemetrySubmissionPeriodical) which is executed every day (to avoid sending excessive telemetry events) to submit metrics (if they are present).

For this, it provides the ability to bind simple suppliers (implemented as subclasses of TelemetryMetricSupplier) which create (optional) metrics for each invocation. If none are registered or none of them returns actual metrics, metrics submission is skipped. Each supplier is registered with a (unique) key, which is used as the event id for the captured event.

A simple supplier could be registered as simple as this:

class SampleModule extends PluginModule {
  protected void configure() {
    addTelemetryMetricProvider("My Metric", () -> Optional.of(TelemetryEvent.of("my_metric", 42)));
  }
}

A supplier does not have to provide a metric, if e.g. the feature was not used:

  addTelemetryMetricProvider("Feature X Metrics", () -> isFeatureUsed ? Optional.of(....) : Optional.empty());

/nocl

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Refactoring (non-breaking change)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.

bernd

Thank you! A few comments.

bernd · 2024-05-06T16:26:14Z

...og2-server/src/main/java/org/graylog2/telemetry/scheduler/TelemetrySubmissionPeriodical.java

+            final var telemetryMetrics = metricsProviders.entrySet()
+                    .stream()
+                    .collect(Collectors.toMap(Map.Entry::getKey, entry -> entry.getValue().get()));
+            telemetryMetrics.forEach((key, value) -> value.map(TelemetryEvent::metrics)
+                    .ifPresent(metrics -> telemetryClient.capture(key, metrics)));


Should we catch any exception here and log a good message instead of letting the periodical system log a generic error message?

...og2-server/src/main/java/org/graylog2/telemetry/scheduler/TelemetrySubmissionPeriodical.java

bernd · 2024-05-06T16:42:29Z

pom.xml

@@ -490,6 +491,11 @@
                <artifactId>auto-value-javabean</artifactId>
                <version>${auto-value-javabean.version}</version>
            </dependency>
+            <dependency>
+                <groupId>com.posthog.java</groupId>
+                <artifactId>posthog</artifactId>


The library looks a bit funky. Using System.out.println and e.printStackTrace() for logging, for example. 😬

https://github.com/PostHog/posthog-java/blob/e230c6417088d26a03c00905d3c301b341eff624/posthog/src/main/java/com/posthog/java/HttpSender.java#L98

I wonder if it's worth including or if we should rather implement our own small client based on our OkHttp client. Not sure how much work the client saves us. I don't think we need the client's queueing mechanism.

I see no way to specify a proxy for the client, so that is already a good reason to roll our own, unfortunately. My only concern are future changes that we cannot mitigate by just bumping the client, but maybe at that point the client is in a better shape, so we can switch.

Thanks!

Their HTTP API should be rather stable. If that's not the case, it would be pretty painful for everyone using the product. 😄

bernd · 2024-05-06T16:45:03Z

graylog2-server/src/main/java/org/graylog2/telemetry/client/TelemetryClient.java

+        this.posthog = () -> new PostHog.Builder(telemetryConfiguration.getTelemetryApiKey())
+                .host(telemetryConfiguration.getTelemetryApiHost())
+                .build();


If we stick with the client (see comment in pom.xml below), we need to set the proxy server if it's configured in the server config.

bernd

Thanks for replacing the lib! 🙏

Two comments below.

bernd · 2024-05-07T07:41:14Z

graylog2-server/src/main/java/org/graylog2/telemetry/client/PosthogAPI.java

+        }
+    }
+
+    @POST


Is the /batch path missing here?

bernd · 2024-05-07T07:48:15Z

graylog2-server/src/main/java/org/graylog2/telemetry/client/TelemetryClient.java

+                    .map(entry -> PosthogAPI.Event.create(clusterId, entry.getKey(), entry.getValue().metrics()))
+                    .toList();
+            final var request = new PosthogAPI.BatchRequest(apiKey, batch);
+            posthog.batchSend(request);


Let's handle response codes and errors here. (https://posthog.com/docs/api/post-only-endpoints#responses)

bernd

Thanks for addressing my comments. I didn't test the code, but the approach looks good! 👍

bernd · 2024-05-08T14:49:14Z

...og2-server/src/main/java/org/graylog2/telemetry/scheduler/TelemetrySubmissionPeriodical.java

+
+    @Override
+    public boolean startOnThisNode() {
+        return true;


I just noticed: we can do the telemetryClient.isEnabled() call here so the periodical doesn't even run if telemetry is disabled.

Fixed that. Had to rebase the branch unfortunately, messed up a merge.

janheise

LGTM!

dennisoelkers requested a review from a team May 6, 2024 09:24

janheise self-requested a review May 6, 2024 12:00

bernd reviewed May 6, 2024

View reviewed changes

bernd requested changes May 7, 2024

View reviewed changes

dennisoelkers requested a review from bernd May 7, 2024 08:09

bernd approved these changes May 7, 2024

View reviewed changes

dennisoelkers marked this pull request as ready for review May 8, 2024 08:02

bernd reviewed May 8, 2024

View reviewed changes

dennisoelkers added 13 commits May 8, 2024 17:05

Adding posthog java library, setting up client.

4b3cb86

Adding capture method, adding cluster id.

0416667

Capture only if telemetry is enabled.

e56dbfd

Establishing periodical and bindings for metrics suppliers.

c62fa95

Make generated TelemetryEvent optional.

91c535b

Adding convenience methods.

228ac05

Adding license headers.

6e7f32b

Construct Posthog client for each submission, shutdown after.

8a112fe

Replacing posthog client with simple homebrewn client.

15f1e10

Adding path to Retrofit interface method.

4af38ec

Handle errors while submitting events.

4b03477

Do not run periodial if telemetry is disabled.

b45bdd7

Do not run capture if metrics are empty.

05b1c4c

dennisoelkers force-pushed the feat/backend-telemetry-setup branch from c535e9c to 05b1c4c Compare May 8, 2024 15:05

dennisoelkers added 2 commits May 13, 2024 13:11

Merge branch 'master' into feat/backend-telemetry-setup

5804831

Including cluster id in groups and separate field.

adaeaa5

janheise approved these changes May 14, 2024

View reviewed changes

janheise merged commit b569b1a into master May 14, 2024
5 checks passed

janheise deleted the feat/backend-telemetry-setup branch May 14, 2024 09:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding simple support for backend telemetry. #19257

Adding simple support for backend telemetry. #19257

dennisoelkers commented May 6, 2024 •

edited

bernd left a comment

bernd May 6, 2024

bernd May 6, 2024

dennisoelkers May 7, 2024

bernd May 7, 2024

bernd May 6, 2024

bernd left a comment

bernd May 7, 2024

dennisoelkers May 7, 2024

bernd May 7, 2024

dennisoelkers May 7, 2024

bernd left a comment

bernd May 8, 2024

dennisoelkers May 8, 2024

janheise left a comment

+                      }
+                  }
+                  @POST

Adding simple support for backend telemetry. #19257

Adding simple support for backend telemetry. #19257

Conversation

dennisoelkers commented May 6, 2024 • edited

Description

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

bernd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bernd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bernd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janheise left a comment

Choose a reason for hiding this comment

dennisoelkers commented May 6, 2024 •

edited