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 collector] add missing struct fields so that payload.meta.browser fields are not lost #1824

Merged
merged 10 commits into from
Oct 7, 2024

Conversation

elee1766
Copy link
Contributor

@elee1766 elee1766 commented Oct 3, 2024

PR Description

a few fields are missing in the struct which is used to read the meta.browser object that faro sends, which results in that information being lost. https://github.com/grafana/faro-web-sdk/blob/v1.10.2/packages/core/src/metas/types.ts#L60-L70

Which issue(s) this PR fixes

fixes #1832

Notes to the Reviewer

i did not do the work for properly parsing NavigatorUABrandVersion[] | string, since it's more work, and i mostly really just want viewportWidth and viewportHeight. since that field is already being ignored right now, it should be safe to not do until maybe someone else who needs it can add it.

PR Checklist

  • CHANGELOG.md updated
  • Documentation added
  • Tests updated
  • Config converters updated

@elee1766 elee1766 requested a review from a team as a code owner October 3, 2024 20:36
@CLAassistant
Copy link

CLAassistant commented Oct 3, 2024

CLA assistant check
All committers have signed the CLA.

@elee1766 elee1766 changed the title [faro collector] add missing fields sent in meta.browser so they are not lost [faro collector] add missing struct fields so that meta.browser fields are not lost when parsing json Oct 3, 2024
@elee1766 elee1766 changed the title [faro collector] add missing struct fields so that meta.browser fields are not lost when parsing json [faro collector] add missing struct fields so that meta.browser fields are not lost Oct 3, 2024
@elee1766 elee1766 changed the title [faro collector] add missing struct fields so that meta.browser fields are not lost [faro collector] add missing struct fields so that payload.meta.browser fields are not lost Oct 3, 2024
@elee1766
Copy link
Contributor Author

elee1766 commented Oct 3, 2024

i've modified the test payload to include some values and added them to the expected test result.

i dont think i have to modify any config adapters or documentation? if i do please let me know

@wildum
Copy link
Contributor

wildum commented Oct 4, 2024

@t00mas do you have some context for this PR?

@t00mas
Copy link

t00mas commented Oct 4, 2024

No context, but LGTM

I'd like to have an issue created, so I can reference that in other components too, please.

@@ -50,6 +50,7 @@ Main (unreleased)

- Fix issue where `loki.source.kubernetes` took into account all labels, instead of specific logs labels. Resulting in duplication. (@mattdurham)

- Fix an issue where some `faro.receiver` would drop multiple fields defined in `payload.meta.browser`, as fields were defined in the struct
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: you can add your github @ if you want credit for it

Copy link
Contributor

@wildum wildum left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@elee1766
Copy link
Contributor Author

elee1766 commented Oct 4, 2024

hi. sorry the context is i wanted the fields so i opened a PR.

i opened a basic issue.
#1832

@wildum wildum merged commit ff673a9 into grafana:main Oct 7, 2024
15 checks passed
@ptodev ptodev mentioned this pull request Oct 7, 2024
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.

[faro collector] payload.meta.browser fields are not decoded and forwarded.
4 participants