[receiver/faroreceiver] Add Faro receiver logic to wireframe by t00mas · Pull Request #39588 · open-telemetry/opentelemetry-collector-contrib

t00mas · 2025-04-23T14:56:41Z

Description

Faro receiver package enables Faro-compatible endpoints. This makes it work.

Link to tracking issue

Fixes #19180

Testing

Adding more unit tests.

t00mas · 2025-04-23T14:57:59Z

Note: I'm basically copying #38224 from @dark0dave and making updates

atoulme · 2025-04-24T06:01:51Z

Moving to draft while this PR is being worked on. Please mark ready to review and ping @dehaansa when done.

atoulme · 2025-04-29T17:17:22Z

@dehaansa please review

dehaansa · 2025-05-01T04:45:58Z

+	var err error
+	receiver := receivers.GetOrAdd(
+		fCfg,
+		func() component.Component {
+			var rcv component.Component
+			rcv, err = newFaroReceiver(fCfg, &set)
+			return rcv
+		},
+	)
+	if err != nil {
+		return nil, err
+	}


This should be refactored into a function.

Refactoring 👍🏼

dehaansa · 2025-05-01T04:51:16Z

+}
+
+func (r *faroReceiver) handleFaroRequest(resp http.ResponseWriter, req *http.Request) {
+	resp.Header().Set("Access-Control-Allow-Methods", "POST, OPTIONS")


Should these only be set in the preflight request response path?

Harmless, but moving these to the preflight path below it 👍🏼

dehaansa · 2025-05-01T04:57:42Z

+		if err == nil {
+			err = r.nextTraces.ConsumeTraces(req.Context(), traces)
+			if err != nil {
+				errors = append(errors, fmt.Sprintf("failed to push traces: %v", err))
+			}
+		} else {
+			r.settings.Logger.Debug("Faro traces are nil, skipping")


I'm confused here - if err != nil we log that "traces are nil", but we haven't actually checked that. Additionally, while the current implementation of the translator might always return a nil err we shouldn't assume that given that err is part of the function definition, we should handle this like a normal function that can return an err.

I think I messed up with a linter error here and removed the check for traces (and logs) - I'm putting it back now

dehaansa · 2025-05-01T04:57:58Z

+		if err == nil {
+			err = r.nextLogs.ConsumeLogs(req.Context(), logs)
+			if err != nil {
+				errors = append(errors, fmt.Sprintf("failed to push logs: %v", err))
+			}
+		} else {
+			r.settings.Logger.Debug("Faro logs are nil, skipping")
+		}


Same as above.

dehaansa · 2025-05-01T04:58:25Z

+			r.settings.Logger.Debug("Faro traces are nil, skipping")
+		}
+	} else {
+		r.settings.Logger.Debug("Traces consumer not registered, skipping")


This could be a noisy debug log, and I'm not sure it provides value.

dehaansa · 2025-05-01T04:59:25Z

+		if err == nil {
+			err = r.nextTraces.ConsumeTraces(req.Context(), traces)
+			if err != nil {
+				errors = append(errors, fmt.Sprintf("failed to push traces: %v", err))


nit : "push" sounds like an exporter action

Suggested change

errors = append(errors, fmt.Sprintf("failed to push traces: %v", err))

errors = append(errors, fmt.Sprintf("failed to pass traces to next consumer: %v", err))

Applying suggestion (sorry for not accepting this one straight away, modified this along the other changes from above)

dehaansa · 2025-05-01T04:59:59Z

+		if err == nil {
+			err = r.nextLogs.ConsumeLogs(req.Context(), logs)
+			if err != nil {
+				errors = append(errors, fmt.Sprintf("failed to push logs: %v", err))


nit : "push" sounds like an exporter action

Suggested change

errors = append(errors, fmt.Sprintf("failed to push logs: %v", err))

errors = append(errors, fmt.Sprintf("failed to pass logs to next consumer: %v", err))

dehaansa · 2025-05-01T05:00:54Z

+	}
+
+	if len(errors) > 0 {
+		r.settings.Logger.Error("Failed to process Faro payload", zap.Any("payload", payload), zap.Any("errors", errors))


Given that clients may re-send, should we fail fast? IE if traces fails, do we want to try to send logs?

I think yes - I'm making this fail fast so not to waste time when things go wrong.

dehaansa · 2025-05-01T05:05:49Z

+	farotranslator "github.com/open-telemetry/opentelemetry-collector-contrib/pkg/translator/faro"
+)
+
+const faroPath = "/"


if I understand the faro spec correctly, should this support receiving data on /collect/{appKey}?

I think the spec needs to be updated, I was assuming anyone was free to receive data on any URL they wish to - @mar4uk ?

agree, let's change the spec. It should be possible to use any URL

dehaansa · 2025-05-01T05:06:37Z

+	resp.WriteHeader(http.StatusOK)
+}
+
+func (r *faroReceiver) errorHandler(w http.ResponseWriter, _ *http.Request, errMsg string, statusCode int) {


From the spec, shouldn't this be json encoded?

Yep! Modifying that
I'm not sure about the schema being right in the spec though - @mar4uk I think the spec mixes Faro payload Exceptions with what the receiver can return

Oh, right, the schema is not correct. It indeed mixes Faro payload Exceptions with error that actually returned

atoulme · 2025-05-02T06:17:17Z

Moving to draft, please fix conflicts and address review and mark ready for review again.

dehaansa

New concern out of the refactoring.

dehaansa · 2025-05-06T17:29:35Z

+	}
+
+	if err := json.NewEncoder(w).Encode(resp); err != nil {
+		r.settings.Logger.Error("Could not write JSON response",


If it fails to write should we default to a precalculated "json" string? Otherwise this seems like it's not responding at all.

Isn't it overkill? json.NewEncoder(w).Encode() only fails (I think) if writing to a closed connection, mem corruption, system-level I/O errors...

A fallback like

fallback := fmt.Sprintf(`{"error":%q,"code":%d,"message":%q}`, http.StatusText(statusCode), statusCode, errMsg)

is easy to add now, but I don't see much benefit.

I hear that, it feels strange handling an error case and not explicitly managing that error. A comment in the code to that effect would be nice for now, in case this ends up being an unexpected issue in the future.

dehaansa

LGTM.

…lemetry#39588)  #### Description Faro receiver package enables Faro-compatible endpoints. This makes it work.  #### Link to tracking issue Fixes open-telemetry#19180  #### Testing Adding more unit tests.

…lemetry#39588)  #### Description Faro receiver package enables Faro-compatible endpoints. This makes it work.  #### Link to tracking issue Fixes open-telemetry#19180  #### Testing Adding more unit tests.

ElementalWarrior · 2025-07-01T17:50:44Z

+receivers:
+  faro:
+    endpoint: 'localhost:8081'
+    cors:


I believe this is incorrect. his should be something like:

receivers: faro: protocols: http: endpoint: 'localhost:8081' cors: ..

Actually I'm not even sure the cors implementation works at all. Both the config I've posted above and the example in the readme do not change the cors response headers.

t00mas requested review from a team and dehaansa as code owners April 23, 2025 14:56

github-actions Bot assigned MovieStoreGuy Apr 23, 2025

github-actions Bot added receiver/faro cmd/otelcontribcol otelcontribcol command labels Apr 23, 2025

github-actions Bot requested review from mar4uk and rlankfo April 23, 2025 14:57

atoulme marked this pull request as draft April 24, 2025 06:01

t00mas force-pushed the faro-receiver branch from 762a812 to 7fda022 Compare April 24, 2025 12:49

t00mas marked this pull request as ready for review April 24, 2025 15:01

github-actions Bot assigned ChrsMark Apr 24, 2025

mar4uk reviewed Apr 25, 2025

View reviewed changes

Comment thread receiver/faroreceiver/utils.go Outdated

t00mas force-pushed the faro-receiver branch 2 times, most recently from 9457ef0 to 7fd4f88 Compare April 25, 2025 15:23

mar4uk approved these changes Apr 28, 2025

View reviewed changes

dehaansa reviewed May 1, 2025

View reviewed changes

atoulme marked this pull request as draft May 2, 2025 06:16

t00mas force-pushed the faro-receiver branch 2 times, most recently from c48b82a to 666bb10 Compare May 2, 2025 13:53

t00mas marked this pull request as ready for review May 2, 2025 14:36

github-actions Bot assigned atoulme May 2, 2025

t00mas force-pushed the faro-receiver branch 4 times, most recently from adbdda5 to f3938a2 Compare May 6, 2025 14:58

[receiver/faroreceiver] Add Faro receiver logic to wireframe

0940c82

t00mas added 17 commits May 6, 2025 17:38

minor README.md fix

ac84ddd

update go.[mod|sum]

9ff9a35

update go.[mod|sum]

648c87e

make goporto

aae5470

lint / headers

e365bf2

add changelog yaml

1c07762

make crosslink

dda340f

lint

cad3a00

remove unused

7fe0d46

go mod tidy

fcea864

make crosslink & go mod tidy

ced59c4

review comments & lints

f72b2e6

remove deprecated config in tests

8d3979d

lint

14d2380

go mod tidy again

454261f

collector module version update

aacbac1

collector module versions & go mod tidy

2c7a0dc

t00mas force-pushed the faro-receiver branch from 1b1072a to 2c7a0dc Compare May 6, 2025 15:38

dehaansa reviewed May 6, 2025

View reviewed changes

dehaansa approved these changes May 7, 2025

View reviewed changes

dehaansa added the ready to merge Code review completed; ready to merge by maintainers label May 7, 2025

atoulme approved these changes May 8, 2025

View reviewed changes

atoulme merged commit a7884df into open-telemetry:main May 8, 2025
182 checks passed

github-actions Bot added this to the next release milestone May 8, 2025

mar4uk mentioned this pull request May 15, 2025

feat: Add faro receiver #38224

Closed

ElementalWarrior reviewed Jul 1, 2025

View reviewed changes

	errors = append(errors, fmt.Sprintf("failed to push traces: %v", err))
	errors = append(errors, fmt.Sprintf("failed to pass traces to next consumer: %v", err))

Conversation

t00mas commented Apr 23, 2025

Description

Link to tracking issue

Testing

Uh oh!

t00mas commented Apr 23, 2025

Uh oh!

atoulme commented Apr 24, 2025

Uh oh!

Uh oh!

atoulme commented Apr 29, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

atoulme commented May 2, 2025

Uh oh!

dehaansa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

t00mas May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dehaansa May 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dehaansa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

t00mas May 6, 2025 •

edited

Loading

dehaansa May 7, 2025 •

edited

Loading