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

[Parser]: add additionnal data in the same log #1819

Open
1 task done
Nightbr opened this issue Apr 18, 2024 · 2 comments
Open
1 task done

[Parser]: add additionnal data in the same log #1819

Nightbr opened this issue Apr 18, 2024 · 2 comments

Comments

@Nightbr
Copy link

Nightbr commented Apr 18, 2024

Hello there 👋 ,

first of all, thank you for this amazing project that is well design and beloved by us (and others for sure ❤️ )

Is there an existing issue that is already proposing this?

  • I have searched the existing issues

Is your feature request related to a problem? Please describe it

We would like to add tenantId in OgmaLogger Interceptor (GraphQL and Express one) to be able to filters in our logs tool (Datadog) and create some dashboards to see the requests per tenantId.
To do this, we need to have this data in the logs of the request.

Describe the solution you'd like

We saw and play with extending the Parser and then use the getMeta from the doc here but it creates a new log that doesn't contains information on the request, the time, ...

Ideally, we would like to have one log per request but be able to add additional properties such as tenantId.

The extends pattern is good, a factory could be great to inject service (ContextService) that will set extra properties. But the main point is to be able to have one log entry.

Teachability, documentation, adoption, migration strategy

  • We could keep the getMeta for extra logs and create a new extraProperties method to handle this use-case
  • We could BC and don't create an extra log with getMeta
  • We could add an extra optional param for getMeta - shouldCreateExtraLog = true default to true

What is the motivation / use case for changing the behavior?

We think this is a must have changes for any multi-tenant apps that wants to have better observability per tenant. This could also be great to easily enhanced and add more custom properties to the request log (userId to see activity per user, apiKeyId same but scoped per apiKey).

@jmcdo29
Copy link
Owner

jmcdo29 commented Jun 25, 2024

First, thank you for the support and patience as I've been dealing with a bunch of life stuff and this ended up getting swallowed by notifications! Really appreciate the feedback and ideas 😄

So, in the JSON format, I can see this being a pretty easy addition as it's just an additional key and value. I'm wondering if I can figure out a way to add it to the string format as well, so that it can be added like the correlation id or the context in a specified format. Might not matter much for local environments, but I feel like it might be a nice way to have it set.

Let me think on what can be done. I might be able to find some way to update the meta or correlation id logging to include this new field.

I assume this would only be done with the RequestScopedLogger when it's not being handled by the interceptor, as it would change per request, correct?

@Nightbr
Copy link
Author

Nightbr commented Jun 25, 2024

It is only for the OgmaLogger Interceptor (we use the GraphQL and Express one). We have a service (let's call it ContextService) that contains all informations such as tenantId and for logs generated by Interceptor (GraphQL or Express) we would like to add this tenantId in the same logs to be able to map it and filter it on logs tool such as Datadog.

The problem is the getMeta from the doc here is creating a new log line and it is not easy to reconciliate both log line.

I would love to be able to add extra data on the interceptor log on the same log line and not create a new line for these extra data. For the JSON format, it's pretty easy as we can do with the already existing meta property. For console, I think like meta, I'm okay to not see it I would say. If we could find a way to inline them, why not but not high priority IMO.

Hope these clarification could help, let me know if you need some help or anything on this!

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

No branches or pull requests

2 participants