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

feat(instrument): stop returning error with instrument functions #229

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

keiko713
Copy link
Contributor

@keiko713 keiko713 commented Apr 7, 2021

While using instrument in identeer, I realized that I had to do the error check every time I call the tracking.
https://github.com/netlify/identeer/pull/96/files#r602408645
This is not a user friendly shape, and also the user could only do the "logging the failure" really, nothing more. With this PR, netlify-commons will do the logging part for you, so that you can use instrument as a one liner.

reference: Segment would return the error with the tracking for the following case, which should be okay to "ignore":

// The method returns an error if the message queue not be queued, which
// happens if the client was already closed at the time the method was
// called or if the message was malformed.

BREAKING CHANGES:
Functions with instrument like instrument.Identify, instrument.Track will no longer return error. If you were using this before with the error handling, please remove the error handling.

@keiko713 keiko713 added the type: feature code contributing to the implementation of a feature and/or user facing functionality label Apr 7, 2021
@keiko713 keiko713 requested a review from a team as a code owner April 7, 2021 15:49
Copy link
Member

@rybit rybit left a comment

Choose a reason for hiding this comment

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

Couple notes:

  • should the errors be logged at Info level? I think Warning could work.
  • should we log out more information, like the userid or traits?
  • I think that if you start calling the global methods without calling Init you're going to panic (b/c the MockClient has a nil Logger). I think that we should not do this, it is especially annoying for tests.

@keiko713
Copy link
Contributor Author

keiko713 commented Apr 8, 2021

should the errors be logged at Info level? I think Warning could work.

yeah good point. I think the given the nature of errors, feels like the most cause is "you're doing something wrong" (client is closed early, or malformed input), so I think it can justify the warning.

should we log out more information, like the userid or traits?

I'm having a bit mixed feeling about this; what are we trying to do with these info? Like, we want to backfill manually? I think we should be able to treat this more with "best effort reporting" than "we can't miss any message" situation.
If the error happens due to some malformed input, some info would help for debugging... are you thinking from that perspective?

I think that if you start calling the global methods without calling Init you're going to panic (b/c the MockClient has a nil Logger). I think that we should not do this, it is especially annoying for tests.

I do have a doc explaining how you use this with test:

For testing, you can create your own mock instrument and use it:
func TestSomething (t *testing.T) {
log := testutil.TL(t)
old := instrument.GetGlobalClient()
t.Cleanup(func(){ instrument.SetGlobalClient(old) })
instrument.SetGlobalClient(instrument.MockClient{Logger: log})
but yeah also nobody reads docs in reality.
I'll see what I can tweak here.

@github-actions github-actions bot added the stale label Mar 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale type: feature code contributing to the implementation of a feature and/or user facing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants