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

Merge meta dictionary to root of body #1

Closed
wants to merge 3 commits into from

Conversation

dgimb89
Copy link

@dgimb89 dgimb89 commented Oct 27, 2020

Merging the meta dictionary on top-level of the body dictionary allows for more structural freedom of the constructed logs. That makes it easier to be consistent with existing projects that log to InsightOps and re-use saved queries across multiple log sets / projects.

In case of conflicts, the proposed implementation gives precendence to the body's values.

…a data on root level

Collisions are resolved by removing the metadata key-value pairs from the metadata map
@ookami-kb
Copy link
Owner

ookami-kb commented Oct 27, 2020

Hi, your point is quite reasonable. However, I'm slightly confused about this assertion/removing keys – looks artificial.

I'm actually thinking about replacing getMeta parameter with something like transformBody with the following signature:

typedef BodyTransformer = Future<Map<String, dynamic>> Function(Map<String, dynamic>);

By default, it will just return the current body (created by the library), at the same time it will allow the client to fully customize the body, using it like this:

InsightOpsLogger(
  _url,
  transformBody: (body) async => {
    ...body,
    'meta': {'deviceId': 'ID'},
  },
);

What do you think, will it solve your use case?

@dgimb89
Copy link
Author

dgimb89 commented Oct 28, 2020

That would definitely work for my use case. I didn't aim for a more generic approach like the one you suggested because I didn't want to change the interface. However, I like your solution better than the one I suggested in this PR.

The key removal makes sure we do not overwrite values defined by the body. The assertion was meant to help users of the package to detect collisions in debug mode without having to look up the fields defined for the body.

Happy to help with the implementation if needed.

@ookami-kb
Copy link
Owner

Cool! Resolved in #2

@ookami-kb ookami-kb closed this Oct 28, 2020
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

2 participants