Skip to content

[receiver/faro] guarantee level is set for faro logs #40701

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 10 commits into from
Jul 10, 2025
Merged

[receiver/faro] guarantee level is set for faro logs #40701

merged 10 commits into from
Jul 10, 2025

Conversation

eskirk
Copy link
Contributor

@eskirk eskirk commented Jun 13, 2025

Description

this PR makes sure that level is set for all logs consumed by the Faro receiver, making sure that the user's settings for logs is maintained.

Link to tracking issue

N/A

Testing

TBD

Documentation

N/A

@eskirk eskirk requested a review from a team as a code owner June 13, 2025 01:03
@eskirk eskirk requested a review from crobert-1 June 13, 2025 01:03
@linux-foundation-easycla Linux Foundation: EasyCLA
Copy link

linux-foundation-easycla bot commented Jun 13, 2025 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

@mar4uk
Copy link
Contributor

mar4uk commented Jun 13, 2025

I think the change should be done in logToKeyVal, exceptionToKeyVal, measurementToKeyVal, eventToKeyVal functions. What we want is to add a missing level field into kv map. The log line is built from kv map:

line, err := logfmt.MarshalKeyvals(keyValToInterfaceSlice(i.kv)...)

So adding level into kvList item will not affect kv map and therefore will not propagate level to the log line

Does it make sense?

mar4uk
mar4uk reviewed Jun 16, 2025
View reviewed changes
Copy link
Contributor

@mar4uk mar4uk left a comment

Choose a reason for hiding this comment

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

looks good! Added a small comment regarding using constants instead of hardcoded strings.
Also changelog entry is needed. The guidance on adding a changelog entry could be found here https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/CONTRIBUTING.md#adding-a-changelog-entry

pkg/translator/faro/keyval.go Outdated
@@ -77,10 +77,17 @@ func keyValToInterfaceSlice(kv *keyVal) []any {
// logToKeyVal represents a Log object as keyVal
func logToKeyVal(l faroTypes.Log) *keyVal {
kv := newKeyVal()

// default to info level, prioritize log level if set
level := "info"
Copy link
Contributor

@mar4uk mar4uk Jun 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Let's use constant faroTypes.LogLevelInfo here and appropriate constants below

@mar4uk
Copy link
Contributor

mar4uk commented Jun 18, 2025

Could you please run the tests locally?make test from the pkg/translator/faro directory. I think they need to be updated because the log line changes

@github-actions github-actions bot requested a review from rlankfo June 25, 2025 22:29
@eskirk eskirk requested a review from mar4uk June 25, 2025 22:47
@eskirk
Copy link
Contributor Author

eskirk commented Jul 1, 2025

@mar4uk can I get an approval for the workflows to run? I fixed tests and I think we are looking good at this point

rlankfo
rlankfo approved these changes Jul 2, 2025
View reviewed changes
Copy link
Member

@rlankfo rlankfo left a comment

Choose a reason for hiding this comment

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

lgtm

@eskirk eskirk requested a review from dehaansa as a code owner July 7, 2025 17:31
dehaansa
dehaansa approved these changes Jul 9, 2025
View reviewed changes
Copy link
Contributor

@dehaansa dehaansa left a comment

Choose a reason for hiding this comment

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

LGTM.

@dehaansa dehaansa added the ready to merge Code review completed; ready to merge by maintainers label Jul 9, 2025
@songy23 songy23 merged commit c10c454 into open-telemetry:main Jul 10, 2025
187 checks passed
@github-actions github-actions bot added this to the next release milestone Jul 10, 2025
Dylan-M pushed a commit to Dylan-M/opentelemetry-collector-contrib that referenced this pull request Aug 5, 2025
@eskirk @Dylan-M
[receiver/faro] guarantee level is set for faro logs (open-telemetry#…
7ed38e6 
…40701)

<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description

this PR makes sure that `level` is set for all logs consumed by the Faro
receiver, making sure that the user's settings for logs is maintained.

<!-- Issue number (e.g. open-telemetry#1234) or full URL to issue, if applicable. -->
#### Link to tracking issue
N/A

<!--Describe what testing was performed and which tests were added.-->
#### Testing
TBD

<!--Describe the documentation added.-->
#### Documentation
N/A
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rlankfo rlankfo rlankfo approved these changes

@dehaansa dehaansa dehaansa approved these changes

@mar4uk mar4uk mar4uk approved these changes

@crobert-1 crobert-1 Awaiting requested review from crobert-1 crobert-1 is a code owner automatically assigned from open-telemetry/collector-contrib-approvers

Assignees

@songy23 songy23

Labels
pkg/translator/faro ready to merge Code review completed; ready to merge by maintainers receiver/faro
Projects
None yet
Milestone
v0.131.0
Development

Successfully merging this pull request may close these issues.
5 participants
@eskirk @mar4uk @rlankfo @dehaansa @songy23