Create a wrapper library to tag span with API metadata #12
Conversation
backend/bufftracer/src/index.ts
Outdated
| const DD_SERVICE_NAME = env.get('DD_SERVICE_NAME').required().asString() | ||
|
|
||
| const DD_TRACE_AGENT_HOST = env | ||
| .get('DD_TRACE_AGENT_HOST') | ||
| .required() | ||
| .asString() | ||
|
|
||
| const DD_TRACE_AGENT_PORT = env | ||
| .get('DD_TRACE_AGENT_PORT') | ||
| .required() | ||
| .asString() | ||
|
|
||
| const DD_ENABLE_TRACING = env.get('DD_ENABLE_TRACING').required().asBool() | ||
| const APP_STAGE = env.get('APP_STAGE').default('').asString() | ||
| const NODE_ENV = env.get('NODE_ENV').asString() |
There was a problem hiding this comment.
@CoxyBoris, I'm thinking maybe we don't rely on ENV var at all and let each client pass these data? I'm not sure what is the best developer experience?
There was a problem hiding this comment.
I got my answer while writing the README, it's much easier to leave that code here, and only ask engineers to set the ENV VAR in the helm chart. The way I wrote the library, we can easily override any of these env variables if we need.
There was a problem hiding this comment.
Couple of comments here, nothing blocking ! Thanks for that work @philippemiguet !
.editorconfig
Outdated
|
|
||
| [*.graphql] | ||
| indent_style = space | ||
| indent_size = 2 |
There was a problem hiding this comment.
Good one, I will try. I had issues with the .md file and copied it from core. Do prettier and eslint work for MB files?
backend/bufftracer/src/types.ts
Outdated
| export function isGraphQlRequest( | ||
| req: GraphQlRequest | RPCRequest, | ||
| ): req is GraphQlRequest { | ||
| // Any GraphQL request (query or mutation) is passed as a string in bode.query | ||
| return req?.body && 'query' in req.body | ||
| } | ||
|
|
||
| export function isRPCRequest( | ||
| req: GraphQlRequest | RPCRequest, | ||
| ): req is RPCRequest { | ||
| const hasNameInBody = req?.body && 'name' in req.body | ||
| const hasMethodInParams = req?.params && 'method' in req.params | ||
| const hasArgsInBody = req?.body && 'args' in req.body | ||
|
|
||
| return (hasNameInBody || hasMethodInParams) && hasArgsInBody | ||
| } |
There was a problem hiding this comment.
Shall these methods leave in this file? 🤔 Sounds more like utils than types 😅
There was a problem hiding this comment.
Yes, if we want types.ts to be purely about types, then yes. I will change that 👍
| span: Span | undefined, | ||
| req: RPCRequest | GraphQlRequest, | ||
| errorCallback: (ex: Error) => void, | ||
| ): boolean { |
There was a problem hiding this comment.
Not sure if it's needed to return anything here 🤔 Looks like it's not used where it's called. Maybe void would be more accurate?
I see that you use it in tests, but maybe we can change that and check if errorCallback was called or not 😇 .
There was a problem hiding this comment.
Great catch, indeed I started returning boolean for the unit tests, but I don't like it! I will change that
| @@ -0,0 +1,12 @@ | |||
| import { GraphQlRequest, RPCRequest } from './types' | |||
|
|
|||
| export const HEADER_CLIENT_ID = 'x-buffer-client-id' | |||
There was a problem hiding this comment.
more of a question, but in datadog will we be able to filter by both the client and the client version? We are currently sending the app version with the client header - just wondering if it will be possible to narrow things down in data dog. E.g "This endpoint hasn't been used since the android app version 6.0, which is from 5 years ago"
There was a problem hiding this comment.
@hitherejoe, the client here represents both the client+version. So, this is how they will appear in DD: https://share.buffer.com/o0uPWg82.
You can then filter by a specific one if you want in a single dropdown (instead of client + version): https://share.buffer.com/4gul6yO4.
Screenshot got taken from there: https://app.datadoghq.com/dashboard/meb-fbz-gmw/buffertools-api-usage-metric?tpl_var_client=%2A&from_ts=1633242996919&to_ts=1633329396919&live=true
If you think it would be better to have two different fields, we can change that 😊
There was a problem hiding this comment.
perfect! Was just curious that's all - I think this will be great as it is for now 🙌
There was a problem hiding this comment.
Looks good to me! Thanks for your work here, @philippemiguet ! 🚀
| tagSpanTrace(span, req) | ||
| }, | ||
| }, | ||
| }) |
There was a problem hiding this comment.
@CoxyBoris, due to a bug within the dd-trace library, we can't export this as an initTracer function to use in our code. If we do, then the tracer.use hook won't work. It seems to be a known issue, so I will open a ticket for DD support and hope to change it back when I get back from vacation.
For now, we will simply do a `import from '@bufferapp/bufftracer' and won't be able to pass an error call back.
There was a problem hiding this comment.
That sounds good to me 🙂 , since the library catch errors and don't crash the process we're good
There was a problem hiding this comment.
Not much to add, but really excited by this work @philippemiguet !
Also since we're pushing tooling to improve devs velocity, and we have issues with old versions of bufflog ) , I'm working on forcing/tracking internal upgrades. Probably a process including depending of dependabot + Jira
New Bufftracer library to standardize how DD traces are tracked and tagged.