add RequestID for each request #543
Conversation
Codecov Report
@@ Coverage Diff @@
## master #543 +/- ##
==========================================
+ Coverage 88.51% 88.55% +0.03%
==========================================
Files 114 114
Lines 3553 3556 +3
==========================================
+ Hits 3145 3149 +4
+ Misses 408 407 -1
Continue to review full report at Codecov.
|
DrMegavolt
force-pushed
the
feature/traceId
branch
from
890858c to
8e1824c
Compare
|
I believe this has become |
|
@altsang Do we want go in this way? Wouldn't it be better to be a core policy — so you pay only for what you use? |
|
yes , i believe that’s fine and also reasonable
i just didn’t want it to be called “trace” anything because that’s not what it’s doing
… On Nov 19, 2017, at 1:53 AM, Vincenzo Chianese ***@***.***> wrote:
@altsang
Just a clarification — this pull request will add this ID for each single request that's hitting the gateway, even if you're not interested in any tracking feature at all.
Do we want go in this way? Wouldn't it be better to be a core policy — so you pay only for what you use?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
core policy like a system wide setting that can be turned on and off? that is fine too
… On Nov 19, 2017, at 1:56 AM, Al Tsang ***@***.***> wrote:
yes , i believe that’s fine and also reasonable
i just didn’t want it to be called “trace” anything because that’s not what it’s doing
> On Nov 19, 2017, at 1:53 AM, Vincenzo Chianese ***@***.***> wrote:
>
> @altsang
> Just a clarification — this pull request will add this ID for each single request that's hitting the gateway, even if you're not interested in any tracking feature at all.
>
> Do we want go in this way? Wouldn't it be better to be a core policy — so you pay only for what you use?
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub, or mute the thread.
>
|
|
@altsang No, we do not have such feature right now. In the current PR the If we make it as a policy, the user would be responsible to put the ID in the request or not. |
|
i rather not make it a policy on its own, it can be part of core and it can
be turned off and on through system.config.yml
way too granular to be a policy on its own, when we introduce a correlation
policy we can add parameters for real tracing use cases including
instrumentation and audit/compliance
this is just a request identifier
…On Sun, Nov 19, 2017 at 2:01 AM Vincenzo Chianese ***@***.***> wrote:
@altsang <https://github.com/altsang> No, we do not have such feature
right now. In the current PR the requestID will always be added, whether
you want it or not.
If we make it as a policy, the user would be responsible to put the ID in
the request or not.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#543 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ACKAcf_v67lzdpe9iRWrnewsAYmutnTFks5s3_x8gaJpZM4Qim_L>
.
|
|
so I looked at this outside of just the Waffle.io comments, it's fine - it adds a variable as part of |
There was a problem hiding this comment.
That looks good to me.
One thing though — I can see you're also modifying the egContext properties as well the way we run code in sandbox. This does not seem to be necessary for this PR (and indeed codeconv is complaining about lack of testing in this part). Is it preparatory for some subsequent PR?
DrMegavolt
changed the title
lib/gateway/context.js
Outdated
|
|
||
| const uuid62 = require('uuid-base62'); | ||
| function EgContextBase () { | ||
| this.requestId = uuid62.v4(); |
There was a problem hiding this comment.
Any linter advice on requestId vs requestID. I tend to follow the latter pattern as that's what built-in JavaScript functions adhere to (e.g., encodeURI).
There was a problem hiding this comment.
ok, ObjectID, MySQL, GraphQL kind of thing.
|
Looks good to me. Just one question on the case of the property name. |
|
That makes sense to me as well.
|
|
Changed to requestID, will update 546 as well |
add requestID for each request
No description provided.