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

Add logs mapper #374

Merged
merged 20 commits into from Jan 16, 2023
Merged

Conversation

louiszawadzki
Copy link
Contributor

@louiszawadzki louiszawadzki commented Jan 10, 2023

What does this PR do?

Add logs event mapper.
If the event mapper returns null, it is dropped.

Additional Notes

We only allow for modification of context and message for now.
We can add support for attributes and user.extraInfo later, but I don't see much usage of this, so let's wait and see if there is a lot of request before adding this.

Review checklist (to be filled by reviewers)

  • Feature or bugfix MUST have appropriate tests
  • Make sure you discussed the feature or bugfix with the maintaining team in an Issue
  • Make sure each commit and the PR mention the Issue number (cf the CONTRIBUTING doc)
  • If this PR is auto-generated, please make sure also to manually update the code related to the change

@louiszawadzki louiszawadzki force-pushed the louiszawadzki/rumm-2585/add-logs-mapper branch 3 times, most recently from e710ca4 to 0df95e8 Compare January 11, 2023 11:10
@louiszawadzki louiszawadzki force-pushed the louiszawadzki/rumm-2585/add-logs-mapper branch 2 times, most recently from cccbc04 to 3f0edc8 Compare January 11, 2023 16:22
@louiszawadzki louiszawadzki force-pushed the louiszawadzki/rumm-2585/add-logs-mapper branch 2 times, most recently from 7e896cc to b9ed21c Compare January 12, 2023 14:26
@louiszawadzki louiszawadzki marked this pull request as ready for review January 12, 2023 14:27
@louiszawadzki louiszawadzki requested a review from a team as a code owner January 12, 2023 14:27
packages/core/src/logs/utils/deepClone.ts Show resolved Hide resolved
packages/core/src/nativeModulesTypes.ts Outdated Show resolved Hide resolved
@louiszawadzki louiszawadzki force-pushed the louiszawadzki/rumm-2585/add-logs-mapper branch from b9ed21c to c4ab3b9 Compare January 13, 2023 09:23
packages/core/src/DdSdkReactNative.tsx Show resolved Hide resolved
InternalLog.log(
`Setting attributes ${JSON.stringify(attributes)}`,
SdkVerbosity.DEBUG
);
return DdSdk.setAttributes(attributes);
await DdSdk.setAttributes(attributes);
AttributesSingleton.getInstance().setAttributes(attributes);
Copy link
Contributor

Choose a reason for hiding this comment

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

There's no return statement anymore, is this normal?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it is intended.
We used to return DdSdk.setAttributes(attributes) which itself returns a Promise<void> (promise resolving a value equal to undefined).
Now that we turned the function into an async one, and used await, it also returns a Promise, which in this case is a Promise<void> as we don't return anything.
This would be equivalent to return; or return undefined but the more common syntax is to not use a return statement.

Copy link
Contributor

Choose a reason for hiding this comment

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

So using async makes an implicit return Promise, I guess this is knowledge I missed. 😅

packages/core/src/logs/eventMapper.ts Show resolved Hide resolved
packages/core/src/logs/eventMapper.ts Show resolved Hide resolved
@louiszawadzki louiszawadzki merged commit b23f4fb into develop Jan 16, 2023
@louiszawadzki louiszawadzki deleted the louiszawadzki/rumm-2585/add-logs-mapper branch January 16, 2023 09:25
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.

None yet

3 participants