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

Faro Exporter structure #24

Merged
merged 3 commits into from
Dec 9, 2024
Merged

Conversation

t00mas
Copy link
Contributor

@t00mas t00mas commented Nov 21, 2024

No description provided.

@t00mas t00mas requested review from rlankfo and mar4uk November 21, 2024 23:00
@t00mas t00mas marked this pull request as ready for review November 25, 2024 14:56

type faroTranslator struct{}

func (t *faroTranslator) TranslateMetrics(_ context.Context, _ pmetric.Metrics) ([]faro.Measurement, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

faro.Measurement is not metric if I'm not mistaken, it is custom signal that is ingested into Loki as log record

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm OK - leaving it out of the exporter currently, since structure-wise it'll probably be a copy of some of the other - what do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

agree 👍

@rlankfo
Copy link
Member

rlankfo commented Nov 27, 2024

Can we move the translator to it's own package? I'd suggest pkg/translator/faro/ to match otel contrib: https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/pkg/translator

This will be used by the receiver as well when we add Faro -> OTLP translations.

@t00mas
Copy link
Contributor Author

t00mas commented Nov 28, 2024

Can we move the translator to it's own package? I'd suggest pkg/translator/faro/ to match otel contrib: https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/pkg/translator

This will be used by the receiver as well when we add Faro -> OTLP translations.

OK moved around the packages so it fits that - reminder that the translator one is scaffolding so I can import something

@t00mas t00mas requested a review from mar4uk November 28, 2024 10:01
)

type FaroClient interface {
SendPayload(payload faro.Payload) error
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think only the SendPayload method should be defined. Because the endpoint accepts faro payload object that contains Logs, Events, Exceptions, Measurements, Traces, etc. I don't think we should split the payload and send all the custom signals separately. Wdyt? Or we can keep the current skeleton, merge it, and tweak its methods later during the implementation

@mar4uk mar4uk self-requested a review December 3, 2024 12:30
@mar4uk mar4uk merged commit ae0605c into grafana:main Dec 9, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants