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

Fix shouldTrace decision based on sampling decision from edge #542

Merged
merged 12 commits into from
Sep 19, 2023
20 changes: 19 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,19 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.

## [Unreleased]

## [6.45.21] - 2023-09-19
### Changed
- Fix tracingMiddleware shouldTrace decision
- Fix HttpClient tracing based on sampling decision

## [6.45.21-beta.2] - 2023-09-19

## [6.45.21-beta.1] - 2023-09-07

## [6.45.21-beta.0] - 2023-09-04

## [6.45.21-beta] - 2023-09-04

## [6.45.20] - 2023-08-30
### Changed
- Remove sampling decision from runtime
Expand Down Expand Up @@ -1776,10 +1789,15 @@ instead
- `HttpClient` now adds `'Accept-Encoding': 'gzip'` header by default.


[Unreleased]: https://github.com/vtex/node-vtex-api/compare/v6.45.20...HEAD
[Unreleased]: https://github.com/vtex/node-vtex-api/compare/v6.45.21...HEAD
[6.45.15]: https://github.com/vtex/node-vtex-api/compare/v6.45.14...v6.45.15
[6.45.14]: https://github.com/vtex/node-vtex-api/compare/v6.45.13...v6.45.14
[6.45.13]: https://github.com/vtex/node-vtex-api/compare/v6.45.12...v6.45.13

[6.45.21]: https://github.com/vtex/node-vtex-api/compare/v6.45.21-beta.2...v6.45.21
[6.45.21-beta.2]: https://github.com/vtex/node-vtex-api/compare/v6.45.21-beta.1...v6.45.21-beta.2
[6.45.21-beta.1]: https://github.com/vtex/node-vtex-api/compare/v6.45.21-beta.0...v6.45.21-beta.1
[6.45.21-beta.0]: https://github.com/vtex/node-vtex-api/compare/v6.45.21-beta...v6.45.21-beta.0
[6.45.21-beta]: https://github.com/vtex/node-vtex-api/compare/v6.45.20...v6.45.21-beta
[6.45.20]: https://github.com/vtex/node-vtex-api/compare/v6.45.20-beta...v6.45.20
[6.45.20-beta]: https://github.com/vtex/node-vtex-api/compare/v6.45.19...v6.45.20-beta
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@vtex/api",
"version": "6.45.20",
"version": "6.45.21",
"description": "VTEX I/O API client",
"main": "lib/index.js",
"typings": "lib/index.d.ts",
Expand Down
20 changes: 10 additions & 10 deletions src/HttpClient/middlewares/cache.ts
Original file line number Diff line number Diff line change
Expand Up @@ -90,13 +90,13 @@ export const cacheMiddleware = ({ type, storage }: CacheOptions) => {
return await next()
}

const span = ctx.tracing!.rootSpan
const span = ctx.tracing?.rootSpan

const key = cacheKey(ctx.config)
const segmentToken = ctx.config.headers[SEGMENT_HEADER]
const keyWithSegment = key + segmentToken

span.log({
span?.log({
event: HttpLogEvents.CACHE_KEY_CREATE,
[HttpCacheLogFields.CACHE_TYPE]: cacheType,
[HttpCacheLogFields.KEY]: key,
Expand All @@ -115,7 +115,7 @@ export const cacheMiddleware = ({ type, storage }: CacheOptions) => {

const now = Date.now()

span.log({
span?.log({
event: HttpLogEvents.LOCAL_CACHE_HIT_INFO,
[HttpCacheLogFields.CACHE_TYPE]: cacheType,
[HttpCacheLogFields.ETAG]: cachedEtag,
Expand All @@ -132,18 +132,18 @@ export const cacheMiddleware = ({ type, storage }: CacheOptions) => {
router: 0,
}

span.setTag(CACHE_RESULT_TAG, CacheResult.HIT)
span?.setTag(CACHE_RESULT_TAG, CacheResult.HIT)
return
}

span.setTag(CACHE_RESULT_TAG, CacheResult.STALE)
span?.setTag(CACHE_RESULT_TAG, CacheResult.STALE)
const validateStatus = addNotModified(ctx.config.validateStatus!)
if (cachedEtag && validateStatus(response.status as number)) {
ctx.config.headers['if-none-match'] = cachedEtag
ctx.config.validateStatus = validateStatus
}
} else {
span.setTag(CACHE_RESULT_TAG, CacheResult.MISS)
span?.setTag(CACHE_RESULT_TAG, CacheResult.MISS)
}

await next()
Expand All @@ -168,7 +168,7 @@ export const cacheMiddleware = ({ type, storage }: CacheOptions) => {
const {forceMaxAge} = ctx.config
const maxAge = forceMaxAge && cacheableStatusCodes.includes(status) ? Math.max(forceMaxAge, headerMaxAge) : headerMaxAge

span.log({
span?.log({
event: HttpLogEvents.CACHE_CONFIG,
[HttpCacheLogFields.CACHE_TYPE]: cacheType,
[HttpCacheLogFields.AGE]: age,
Expand All @@ -182,7 +182,7 @@ export const cacheMiddleware = ({ type, storage }: CacheOptions) => {

// Indicates this should NOT be cached and this request will not be considered a miss.
if (!forceMaxAge && (noStore || (noCache && !etag))) {
span.log({ event: HttpLogEvents.NO_LOCAL_CACHE_SAVE, [HttpCacheLogFields.CACHE_TYPE]: cacheType })
span?.log({ event: HttpLogEvents.NO_LOCAL_CACHE_SAVE, [HttpCacheLogFields.CACHE_TYPE]: cacheType })
return
}

Expand All @@ -207,7 +207,7 @@ export const cacheMiddleware = ({ type, storage }: CacheOptions) => {
responseType,
})

span.log({
span?.log({
event: HttpLogEvents.LOCAL_CACHE_SAVED,
[HttpCacheLogFields.CACHE_TYPE]: cacheType,
[HttpCacheLogFields.KEY_SET]: setKey,
Expand All @@ -221,7 +221,7 @@ export const cacheMiddleware = ({ type, storage }: CacheOptions) => {
return
}

span.log({ event: HttpLogEvents.NO_LOCAL_CACHE_SAVE, [HttpCacheLogFields.CACHE_TYPE]: cacheType })
span?.log({ event: HttpLogEvents.NO_LOCAL_CACHE_SAVE, [HttpCacheLogFields.CACHE_TYPE]: cacheType })
}
}

Expand Down
12 changes: 6 additions & 6 deletions src/HttpClient/middlewares/memoization.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,21 +17,21 @@ export const memoizationMiddleware = ({ memoizedCache }: MemoizationOptions) =>
return next()
}

const span = ctx.tracing!.rootSpan
const span = ctx.tracing?.rootSpan

const key = cacheKey(ctx.config)
const isMemoized = !!memoizedCache.has(key)

span.log({ event: HttpLogEvents.CACHE_KEY_CREATE, [HttpCacheLogFields.CACHE_TYPE]: 'memoization', [HttpCacheLogFields.KEY]: key })
span?.log({ event: HttpLogEvents.CACHE_KEY_CREATE, [HttpCacheLogFields.CACHE_TYPE]: 'memoization', [HttpCacheLogFields.KEY]: key })

if (isMemoized) {
span.setTag(CustomHttpTags.HTTP_MEMOIZATION_CACHE_RESULT, CacheResult.HIT)
span?.setTag(CustomHttpTags.HTTP_MEMOIZATION_CACHE_RESULT, CacheResult.HIT)
const memoized = await memoizedCache.get(key)!
ctx.memoizedHit = isMemoized
ctx.response = memoized.response
return
} else {
span.setTag(CustomHttpTags.HTTP_MEMOIZATION_CACHE_RESULT, CacheResult.MISS)
span?.setTag(CustomHttpTags.HTTP_MEMOIZATION_CACHE_RESULT, CacheResult.MISS)
const promise = new Promise<Memoized>(async (resolve, reject) => {
try {
await next()
Expand All @@ -40,10 +40,10 @@ export const memoizationMiddleware = ({ memoizedCache }: MemoizationOptions) =>
response: ctx.response!,
})

span.log({ event: HttpLogEvents.MEMOIZATION_CACHE_SAVED, [HttpCacheLogFields.KEY_SET]: key })
span?.log({ event: HttpLogEvents.MEMOIZATION_CACHE_SAVED, [HttpCacheLogFields.KEY_SET]: key })
} catch (err) {
reject(err)
span.log({ event: HttpLogEvents.MEMOIZATION_CACHE_SAVED_ERROR, [HttpCacheLogFields.KEY_SET]: key })
span?.log({ event: HttpLogEvents.MEMOIZATION_CACHE_SAVED_ERROR, [HttpCacheLogFields.KEY_SET]: key })
}
})
memoizedCache.set(key, promise)
Expand Down
8 changes: 4 additions & 4 deletions src/HttpClient/middlewares/request/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -65,10 +65,10 @@ export const defaultsMiddleware = ({ baseURL, rawHeaders, params, timeout, retri
}


if(ctx.tracing!.isSampled) {
if(ctx.tracing?.isSampled) {
const { config } = ctx
const fullUrl = buildFullPath(config.baseURL, http.getUri(config))
ctx.tracing!.rootSpan.addTags({
ctx.tracing?.rootSpan?.addTags({
[OpentracingTags.HTTP_METHOD]: config.method || 'get',
[OpentracingTags.HTTP_URL]: fullUrl,
})
Expand All @@ -89,7 +89,7 @@ export const routerCacheMiddleware = async (ctx: MiddlewareContext, next: () =>
const status = ctx.response?.status

if(routerCacheHit) {
ctx.tracing!.rootSpan.setTag(CustomHttpTags.HTTP_ROUTER_CACHE_RESULT, routerCacheHit)
ctx.tracing?.rootSpan?.setTag(CustomHttpTags.HTTP_ROUTER_CACHE_RESULT, routerCacheHit)
}

if (routerCacheHit === ROUTER_CACHE_HIT || (routerCacheHit === ROUTER_CACHE_REVALIDATED && status !== 304)) {
Expand All @@ -106,4 +106,4 @@ export const requestMiddleware = (limit?: Limit) => async (ctx: MiddlewareContex
const makeRequest = () => http.request(ctx.config)

ctx.response = await (limit ? limit(makeRequest) : makeRequest())
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ const onResponseError = (http: AxiosInstance) => (error: any) => {
: config.timeout
config.transformRequest = [data => data]

config.tracing!.rootSpan!.log({ event: HttpLogEvents.SETUP_REQUEST_RETRY, [HttpRetryLogFields.RETRY_NUMBER]: config.retryCount, [HttpRetryLogFields.RETRY_IN]: delay })
config.tracing?.rootSpan?.log({ event: HttpLogEvents.SETUP_REQUEST_RETRY, [HttpRetryLogFields.RETRY_NUMBER]: config.retryCount, [HttpRetryLogFields.RETRY_IN]: delay })

return new Promise(resolve => setTimeout(() => resolve(http(config)), delay))
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ export const requestSpanPrefix = 'http-request'
const preRequestInterceptor = (http: AxiosInstance) => (
config: TraceableAxiosRequestConfig
): TraceableAxiosRequestConfig => {
if (!config.tracing || !config.tracing.isSampled) {
if (!config.tracing || !config.tracing?.isSampled) {
return config
}

Expand All @@ -48,18 +48,18 @@ const preRequestInterceptor = (http: AxiosInstance) => (
}

const onResponseSuccess = (response: TraceableAxiosResponse): TraceableAxiosResponse => {
if (!response.config.tracing || !response.config.tracing.isSampled) {
if (!response.config.tracing || !response.config.tracing?.isSampled) {
return response
}

const requestSpan = response.config.tracing.requestSpan!
const requestSpan = response.config.tracing?.requestSpan
injectResponseInfoOnSpan(requestSpan, response)
requestSpan.finish()
requestSpan?.finish()
return response
}

const onResponseError = (err: ExtendedAxiosError) => {
if (!err?.config?.tracing?.requestSpan || !err.config.tracing.isSampled) {
if (!err?.config?.tracing?.requestSpan || !err.config.tracing?.isSampled) {
return Promise.reject(err)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,26 +5,26 @@ import { ROUTER_CACHE_HEADER } from '../../../../../../constants'
import { CustomHttpTags, OpentracingTags } from '../../../../../../tracing/Tags'
import { cloneAndSanitizeHeaders } from '../../../../../../tracing/utils'

export const injectRequestInfoOnSpan = (span: Span, http: AxiosInstance, config: AxiosRequestConfig) => {
span.addTags({
export const injectRequestInfoOnSpan = (span: Span | undefined, http: AxiosInstance, config: AxiosRequestConfig) => {
span?.addTags({
[OpentracingTags.SPAN_KIND]: OpentracingTags.SPAN_KIND_RPC_CLIENT,
[OpentracingTags.HTTP_METHOD]: config.method,
[OpentracingTags.HTTP_URL]: buildFullPath(config.baseURL, http.getUri(config)),
})

span.log({ 'request-headers': cloneAndSanitizeHeaders(config.headers) })
span?.log({ 'request-headers': cloneAndSanitizeHeaders(config.headers) })
}

// Response may be undefined in case of client timeout, invalid URL, ...
export const injectResponseInfoOnSpan = (span: Span, response: AxiosResponse<any> | undefined) => {
export const injectResponseInfoOnSpan = (span: Span | undefined, response: AxiosResponse<any> | undefined) => {
if (!response) {
span.setTag(CustomHttpTags.HTTP_NO_RESPONSE, 'true')
span?.setTag(CustomHttpTags.HTTP_NO_RESPONSE, 'true')
return
}

span.log({ 'response-headers': cloneAndSanitizeHeaders(response.headers) })
span.setTag(OpentracingTags.HTTP_STATUS_CODE, response.status)
span?.log({ 'response-headers': cloneAndSanitizeHeaders(response.headers) })
span?.setTag(OpentracingTags.HTTP_STATUS_CODE, response.status)
if (response.headers[ROUTER_CACHE_HEADER]) {
span.setTag(CustomHttpTags.HTTP_ROUTER_CACHE_RESULT, response.headers[ROUTER_CACHE_HEADER])
span?.setTag(CustomHttpTags.HTTP_ROUTER_CACHE_RESULT, response.headers[ROUTER_CACHE_HEADER])
}
}
28 changes: 15 additions & 13 deletions src/HttpClient/middlewares/tracing.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { MiddlewaresTracingContext, RequestConfig } from '..'
import { IOContext } from '../../service/worker/runtime/typings'
import { createSpanReference, ErrorReport, getTraceInfo, SpanReferenceTypes } from '../../tracing'
import { ErrorReport, getTraceInfo } from '../../tracing'
import { CustomHttpTags, OpentracingTags } from '../../tracing/Tags'
import { MiddlewareContext } from '../typings'
import { CacheType, isLocallyCacheable } from './cache'
Expand All @@ -25,13 +25,15 @@ export const createHttpClientTracingMiddleware = ({
hasDiskCacheMiddleware,
}: HttpClientTracingMiddlewareConfig) => {
return async function tracingMiddleware(ctx: MiddlewareContext, next: () => Promise<void>) {
const { rootSpan, requestSpanNameSuffix, referenceType = SpanReferenceTypes.CHILD_OF } = ctx.config.tracing || {}
if(!tracer.isTraceSampled){
await next()
return
}

const rootSpan = tracer.fallbackSpanContext()
const { requestSpanNameSuffix } = ctx.config.tracing || {}
const spanName = requestSpanNameSuffix ? `request:${requestSpanNameSuffix}` : 'request'
const span = rootSpan
? tracer.startSpan(spanName, {
references: [createSpanReference(rootSpan, referenceType)],
})
: tracer.startSpan(spanName)
const span = tracer.startSpan(spanName, {childOf: rootSpan})

ctx.tracing = {
...ctx.config.tracing,
Expand All @@ -49,7 +51,7 @@ export const createHttpClientTracingMiddleware = ({
const hasMemoryCache = hasMemoryCacheMiddleware && !!isLocallyCacheable(ctx.config, CacheType.Memory)
const hasDiskCache = hasDiskCacheMiddleware && !!isLocallyCacheable(ctx.config, CacheType.Disk)

span.addTags({
span?.addTags({
[CustomHttpTags.HTTP_MEMOIZATION_CACHE_ENABLED]: hasMemoCache,
[CustomHttpTags.HTTP_MEMORY_CACHE_ENABLED]: hasMemoryCache,
[CustomHttpTags.HTTP_DISK_CACHE_ENABLED]: hasDiskCache,
Expand All @@ -62,19 +64,19 @@ export const createHttpClientTracingMiddleware = ({
response = ctx.response
} catch (err) {
response = err.response
if(ctx.tracing.isSampled) {
if(ctx.tracing?.isSampled) {
ErrorReport.create({ originalError: err }).injectOnSpan(span, logger)
}

throw err
} finally {
if (response) {
span.setTag(OpentracingTags.HTTP_STATUS_CODE, response.status)
span?.setTag(OpentracingTags.HTTP_STATUS_CODE, response.status)
} else {
span.setTag(CustomHttpTags.HTTP_NO_RESPONSE, true)
span?.setTag(CustomHttpTags.HTTP_NO_RESPONSE, true)
}

span.finish()
span?.finish()
}
}
}
2 changes: 1 addition & 1 deletion src/HttpClient/typings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ export interface CacheHit {
export interface MiddlewaresTracingContext extends Omit<RequestTracingUserConfig, 'rootSpan'> {
tracer: IUserLandTracer
logger: IOContext['logger']
rootSpan: Span
rootSpan?: Span
isSampled: boolean
}

Expand Down
2 changes: 1 addition & 1 deletion src/service/logger/logger.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ interface LoggerContext extends Pick<IOContext, 'account'|'workspace'|'requestI

interface TracingState {
isTraceSampled: boolean,
traceId: string
traceId?: string
}

export class Logger {
Expand Down
Loading