[ECI-466] Metric Forwarding In Go #40

sva91 · 2025-02-12T21:58:22Z

What

-Write Metrics Forwarder in Go

Steps to build multi-architecture image

Enable buildx : docker buildx create --use
Build and push image : docker buildx build --platform linux/amd64,linux/arm64 -t iad.ocir.io/iduhc9hzgn3o/metrics-forwarder-function/metrics-forwarder:0.0.3 . --no-cache --push

Why

Onboarding improvements

Testing

Unit tests
Staging Logs
Using the same multi-platform image with function application of ARM and x86 architectures
Invoked both the images through connector hub and it works fine

metrics-forwarder/forwarder.go

maxpyf · 2025-02-13T14:06:36Z

metrics-forwarder/Dockerfile

+COPY . . 
+
+# Enable Go modules & build with optimizations
+RUN go mod tidy && \


Do we need to support X86 as well?
I can't remember if OCI in Japan supports only ARM or only X86?

Japan only supports x86 but I think we said with Kanishk's stack improvements we won't need to worry about it later on

well... we don't need to worry about it if we build for X86 and have multi arch images.
So I would say here we do need to build it as both.

I have updated the image to build for both platforms. Also updated the PR description to have instructions on how to build such an image, and the testing I have done to ensure the same image works on function application of both architectures.

Agreed, the multi-arch image build is managed outside the docker file using buildx so that will be a separate step

maxpyf · 2025-02-13T14:09:24Z

metrics-forwarder/forwarder.go

+}
+
+func sendMetricsToDatadog(metricsMessage []byte, client HTTPClient) error {
+	endpoint := os.Getenv("DD_INTAKE_HOST")


I see that this was the way in the Python version as well, so we can go with this for now...
Should we consider the DD API key a secret?
From our perspective maybe we don't care (i.e. we charge more if someone sends more stuff to us) but from the customer's standpoint?
Anyway not a blocker for this PR by any means.

I think DD API Key should be considered a secret, but I don't fully follow what changes are you recommending based on that. Are you suggesting that this should not be a configuration for function application, but sourced from somewhere else (OCI vault etc.) ?

That is what I am saying, but of course it is not for this PR and maybe not even something we can/should do at all.
I am not recommending any changes today, but I think we should think about it as a team for the future.

Yea, I definitely agree that this may require some discussion, as well as some experimentation. In my opinion, it is certainly a good idea to add API-KEY and APP-KEY to the customer's vault during initial stack execution, and replicate it in all regions at that time. We can then use it everywhere we need this.
I think this will be really nice for longer term. We do have to think about how to handle key rotations on expiration.

maxpyf · 2025-02-13T14:10:39Z

metrics-forwarder/forwarder.go

+	if shouldCompressPayload() {
+		compressedPayload, err := compressPayload(metricsMessage)
+		if err != nil {
+			log.Printf("Error compressing payload: %v", err)


Also not a blocker for this version, but ultimately I think we should report this kind of error to us via the EvP endpoint, so that we can monitor when errors occur in the fleet and fix them proactively.

That makes complete sense, but I think that should be a separate PR with focus on "relaying information from forwarders to Datadog", so that we can think about holistically.

metrics-forwarder/forwarder.go

metrics-forwarder/formatter.go

metrics-forwarder/func.yaml

metrics-forwarder/func_test.go

metrics-forwarder/forwarder_test.go

metrics-forwarder/func_test.go

bengillmandd

overall lgtm just had some nitpicks

metrics-forwarder/Dockerfile

metrics-forwarder/forwarder.go

metrics-forwarder/func.go

metrics-forwarder/response.go

metrics-forwarder/formatter.go

metrics-forwarder/forwarder.go

metrics-forwarder/func.go

kanishktripathi · 2025-02-19T21:29:42Z

metrics-forwarder/internal/client/client.go

+
+func CreateDatadogClient(site, apiKey string) DatadogClient {
+	configuration := datadog.NewConfiguration()
+	configuration.RetryConfiguration.EnableRetry = true


Do we want to set max Retries and the retry timeout?

Default setting for these values are as follows (Link):

RetryConfiguration: RetryConfiguration{ EnableRetry: false, BackOffMultiplier: 2, BackOffBase: 2, HTTPRetryTimeout: 60 * time.Second, MaxRetries: 3, }

If we want to change them, we can override, but they looked fine to me as is.

kanishktripathi · 2025-02-19T22:18:18Z

metrics-forwarder/internal/client/forwarder.go

+		return err
+	}
+	if resp.StatusCode < 200 || resp.StatusCode >= 300 {
+		log.Printf("Error: Received non-200 response from Datadog: %d", resp.StatusCode)


Do we use log or fmt here considering we're using fmt to print the logs in other places. Same in handler.go

Great question. I experimented with both. Here is how these different log packages are treated in OCI.

Log package : These are written to STDERR by default. You will see "src" : "STDERR" in the log json.

fmt package : These are written to STDOUT by default. You will see "src" : "STDOUT" in the log json.

If a log is an error, I used the log package, else I used the fmt package.

kanishktripathi · 2025-02-19T22:32:56Z

metrics-forwarder/internal/handler/handler.go

+	"metrics-forwarder/internal/formatter"
+)
+
+var sendMetricsFunc = client.SendMetricsToDatadog


we can call datadog.NewAPIClient since this client is what is used for connection and as var can be re-used if the container is not destroyed.
Internally we can also decide the size of connection pool with this which is again created once

I am not sure I fully understand this comment. I am happy to chat more about this.

kanishktripathi · 2025-02-19T22:41:04Z

metrics-forwarder/internal/handler/response.go

+	if status == "success" {
+		fdk.WriteStatus(out, 200)
+	} else {
+		fdk.WriteStatus(out, 500)


500 status may not be accurate for scenarios like:

Invalid API key when getting a 403 response from the endpoint

missing API key, site or tenancy OCID (more closer to a 400 error)

Hmm, I am considering the response from function invocations as a pass or fail only. The status code from Datadog itself will be printed in OCI logs.

This particular status code becomes relevant if the function itself is placed behind an API Gateway, and then invoked through an API call. Now, for the client of the function itself, if an internal API call is failing, should we mask that or relay that information as is, could be debated.

Let me know if you want to have a discussion about this.

metrics-forwarder/internal/handler/handler.go

kanishktripathi · 2025-02-19T23:02:03Z

metrics-forwarder/internal/client/forwarder.go

+// Returns:
+//   - error: An error if the request preparation or API call fails, or if the
+//     response status code indicates a failure.
+func SendMetricsToDatadog(client DatadogClient, metricsMessage []byte) error {


can this be a method of DatadogClient ?

I am not sure I fully understand the intent behind making this a function on struct, instead of passing the struct. This way, it helps me test the send function by creating a mock client.

metrics-forwarder/internal/client/client.go

metrics-forwarder/go.mod

sva91 added 2 commits February 12, 2025 16:52

Metric Forwarding In Go

4607ef7

remove binary

c06f98d

sva91 added the eci label Feb 12, 2025

sva91 requested a review from a team February 12, 2025 22:30

sva91 marked this pull request as ready for review February 12, 2025 22:31

sva91 commented Feb 12, 2025

View reviewed changes

metrics-forwarder/forwarder.go Outdated Show resolved Hide resolved

maxpyf reviewed Feb 13, 2025

View reviewed changes

metrics-forwarder/forwarder.go Outdated Show resolved Hide resolved

maxpyf reviewed Feb 13, 2025

View reviewed changes

metrics-forwarder/forwarder.go Outdated Show resolved Hide resolved

rheei reviewed Feb 13, 2025

View reviewed changes

metrics-forwarder/formatter.go Outdated Show resolved Hide resolved

maxpyf reviewed Feb 13, 2025

View reviewed changes

metrics-forwarder/func.yaml Show resolved Hide resolved

maxpyf reviewed Feb 13, 2025

View reviewed changes

metrics-forwarder/func_test.go Outdated Show resolved Hide resolved

rheei reviewed Feb 13, 2025

View reviewed changes

metrics-forwarder/forwarder_test.go Outdated Show resolved Hide resolved

rheei reviewed Feb 13, 2025

View reviewed changes

metrics-forwarder/func_test.go Outdated Show resolved Hide resolved

reviewer comments - part 1

ea7bd7e

bengillmandd reviewed Feb 13, 2025

View reviewed changes

metrics-forwarder/func.go Show resolved Hide resolved

sva91 added 4 commits February 13, 2025 15:59

Use Datadog Client

161d05e

support multi platform images

67a3a98

Reviewer Comments formatter

125ddba

refactor code into packages

c24cb92

sva91 requested a review from bengillmandd February 18, 2025 19:50

bengillmandd approved these changes Feb 18, 2025

View reviewed changes

sva91 changed the title ~~Metric Forwarding In Go~~ [ECI-466] Metric Forwarding In Go Feb 18, 2025

kanishktripathi reviewed Feb 19, 2025

View reviewed changes

reviewer comments

8aab729

datadog-datadog-prod-us1 bot reviewed Feb 20, 2025

View reviewed changes

metrics-forwarder/go.mod Show resolved Hide resolved

metrics-forwarder/go.mod Show resolved Hide resolved

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ECI-466] Metric Forwarding In Go #40

[ECI-466] Metric Forwarding In Go #40

sva91 commented Feb 12, 2025 •

edited

Loading

maxpyf Feb 13, 2025

rheei Feb 13, 2025

maxpyf Feb 13, 2025

sva91 Feb 14, 2025

kanishktripathi Feb 19, 2025

maxpyf Feb 13, 2025

sva91 Feb 13, 2025

maxpyf Feb 13, 2025

sva91 Feb 13, 2025 •

edited

Loading

maxpyf Feb 13, 2025

sva91 Feb 13, 2025

bengillmandd left a comment

kanishktripathi Feb 19, 2025

sva91 Feb 20, 2025

kanishktripathi Feb 19, 2025

sva91 Feb 20, 2025

kanishktripathi Feb 19, 2025

sva91 Feb 20, 2025

kanishktripathi Feb 19, 2025

sva91 Feb 20, 2025 •

edited

Loading

kanishktripathi Feb 19, 2025

sva91 Feb 20, 2025

[ECI-466] Metric Forwarding In Go #40

Are you sure you want to change the base?

[ECI-466] Metric Forwarding In Go #40

Conversation

sva91 commented Feb 12, 2025 • edited Loading

What

Steps to build multi-architecture image

Why

Testing

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sva91 Feb 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bengillmandd 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sva91 Feb 20, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sva91 commented Feb 12, 2025 •

edited

Loading

sva91 Feb 13, 2025 •

edited

Loading

sva91 Feb 20, 2025 •

edited

Loading