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

Initial consumer logic, logs the messages for given topic #2

Merged
merged 17 commits into from
Jul 15, 2019

Conversation

guptad2
Copy link
Contributor

@guptad2 guptad2 commented Jul 10, 2019

Some problem with init-changes. This contents of this branch are the same as init-changes.

Copy link

@pallavi2209 pallavi2209 left a comment

Choose a reason for hiding this comment

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

@guptad2 I notice the test directory named __tests__. Is there a reason for adding underscores? Is that a standard practice for node.js projects? Curious because it looks a bit different than other node.js projects we have.

@guptad2
Copy link
Contributor Author

guptad2 commented Jul 10, 2019

@pallavi2209 The testing framework used here uses this regular expression to detect the test files.

"**/__tests__/**/*.[jt]s?(x)", "**/?(*.)+(spec|test).[jt]s?(x)"

So it was either naming the directory __tests__ or naming files fileName.test.js

messageSet.forEach((m) => {
const key = m.message.key;
const value = m.message.value;
logger.info('Message from topic', topic, key, value);
Copy link
Contributor

Choose a reason for hiding this comment

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

We should consider structuring this output a little better, e.g. both sumo and pino support log lines as json object, and having that kind of structure might be nice to have e.g. set up topic and key as "properties" on the pino log item... ?

Copy link
Contributor

Choose a reason for hiding this comment

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

But we can make that a separate story if you want

Copy link
Contributor Author

@guptad2 guptad2 Jul 11, 2019

Choose a reason for hiding this comment

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

Yea I was thinking to make it a separate story after we can spec out the log format in detail? Because I am not too sure how the later use case would work (storing logs in a big data store + pub/sub aggregation) and I guess the structure of the logs and how we handle them would depend on that?

@guptad2 guptad2 temporarily deployed to refocus-logging-staging July 15, 2019 18:40 Inactive
@guptad2 guptad2 temporarily deployed to refocus-logging-staging July 15, 2019 22:05 Inactive
@guptad2 guptad2 temporarily deployed to refocus-logging-staging July 15, 2019 22:14 Inactive
@guptad2 guptad2 temporarily deployed to refocus-logging-staging July 15, 2019 22:41 Inactive
@iamigo iamigo merged commit 25a42fa into master Jul 15, 2019
@iamigo iamigo deleted the init-consumer branch July 15, 2019 23:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants