Skip to content
This repository has been archived by the owner on Jun 29, 2023. It is now read-only.

Handling Long in GelfUtil.addMdcProfiling #73

Closed
prlj13 opened this issue Mar 22, 2016 · 5 comments
Closed

Handling Long in GelfUtil.addMdcProfiling #73

prlj13 opened this issue Mar 22, 2016 · 5 comments
Labels
type: bug A general bug
Milestone

Comments

@prlj13
Copy link

prlj13 commented Mar 22, 2016

It looks like handling a Long in GelfUtil.addMdcProfiling isn't done properly. There's a couple of ways to fix it. I'd add an else to the requestStartMs instanceof Long (and removing timestamp == -1 is possible too).

Resulting in something like this:
if (instanceof Long) {...} else if (instanceof String) {...} else {return;}

@mp911de
Copy link
Owner

mp911de commented Mar 22, 2016

Could you elaborate what you mean by not properly? The code could be improved but is there a bug?

@prlj13
Copy link
Author

prlj13 commented Mar 22, 2016

If requestStartMs is a Long, which it looks like addMdcProfiling is supposed to support, then the value is ignored and the duration isn't added to the message.

@mp911de
Copy link
Owner

mp911de commented Mar 22, 2016

requestStartMs must be in any case numeric and can be stored within the MDC either as long (Long) or as String. The latter is used with slf4j's MDC. The code is a leftover from direct MDC usage. LogEvent returns directly a String.

You're totally right, the bug is so obvious that I haven't seen it for years now. Do you want to send in a PR or should I take care of it?

@prlj13
Copy link
Author

prlj13 commented Mar 22, 2016

I'm a newbie (not setup with GIT, just reviewing the source), so I'll let you take care of it.

mp911de added a commit that referenced this issue Mar 22, 2016
Remove leftover conversion from direct MDC usage as LogEvent returns MDC values in every case as String.
@mp911de mp911de added the type: bug A general bug label Mar 22, 2016
@mp911de mp911de added this to the 1.9.0 milestone Mar 22, 2016
@mp911de
Copy link
Owner

mp911de commented Mar 22, 2016

Thanks for the bug report, fixed with a916fd1.

@mp911de mp911de closed this as completed Mar 22, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
type: bug A general bug
Projects
None yet
Development

No branches or pull requests

2 participants