Skip to content

Commit

Permalink
Merge branch 'benoit/validate-tags--fix-staging' (87ee856) into stagi…
Browse files Browse the repository at this point in the history
…ng-51

 pm_trace_id: 6351007
 feature_branch_pipeline_id: 6351007
 source: to-staging

* commit '87ee85682347e9d7a8efc46f4b8d7388af82f518':
  ✅ use valid tag values in tests
  👌 simplify implementation
  🔥 [RUMF-1089] Cleanup legacy intake URLs (#1214)
  ✨ [RUMF-827] sanitize tags
  ♻️ [RUMF-827] add function to build tags at a higher level
  • Loading branch information
BenoitZugmeyer committed Dec 15, 2021
2 parents 6802db7 + 87ee856 commit dfa4033
Show file tree
Hide file tree
Showing 8 changed files with 108 additions and 26 deletions.
11 changes: 3 additions & 8 deletions packages/core/src/domain/configuration/endpointBuilder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,11 @@ export function createEndpointBuilder(
initConfiguration: InitConfiguration,
buildEnv: BuildEnv,
endpointType: EndpointType,
tags: string[],
source?: string
) {
const sdkVersion = buildEnv.sdkVersion
const { site = INTAKE_SITE_US, clientToken, env, proxyHost, proxyUrl, service, version } = initConfiguration
const { site = INTAKE_SITE_US, clientToken, proxyHost, proxyUrl } = initConfiguration

const host = buildHost(endpointType)
const path = buildPath(endpointType)
Expand Down Expand Up @@ -62,15 +63,9 @@ export function createEndpointBuilder(
}

function buildQueryParameters(endpointType: EndpointType, source?: string) {
const tags =
`sdk_version:${sdkVersion}` +
`${env ? `,env:${env}` : ''}` +
`${service ? `,service:${service}` : ''}` +
`${version ? `,version:${version}` : ''}`

let parameters =
`ddsource=${source || 'browser'}` +
`&ddtags=${encodeURIComponent(tags)}` +
`&ddtags=${encodeURIComponent([`sdk_version:${sdkVersion}`].concat(tags).join(','))}` +
`&dd-api-key=${clientToken}` +
`&dd-evp-origin-version=${encodeURIComponent(sdkVersion)}` +
`&dd-evp-origin=browser` +
Expand Down
45 changes: 45 additions & 0 deletions packages/core/src/domain/configuration/tags.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
import { display } from '../../tools/display'
import { InitConfiguration } from './configuration'
import { buildTag, buildTags, TAG_SIZE_LIMIT } from './tags'

const LARGE_VALUE = Array(TAG_SIZE_LIMIT + 10).join('a')

describe('buildTags', () => {
it('build tags from init configuration', () => {
expect(
buildTags({
service: 'foo',
env: 'bar',
version: 'baz',
} as InitConfiguration)
).toEqual(['env:bar', 'service:foo', 'version:baz'])
})
})

describe('buildTag', () => {
let displaySpy: jasmine.Spy<typeof display.warn>
beforeEach(() => {
displaySpy = spyOn(display, 'warn')
})

it('lowercases tags', () => {
expect(buildTag('env', 'BaR')).toBe('env:bar')
expectWarning()
})

it('trims large tags', () => {
const tag = buildTag('env', LARGE_VALUE)
expect(tag.length).toBe(TAG_SIZE_LIMIT)
expect(tag).toBe(`env:${LARGE_VALUE.slice(0, TAG_SIZE_LIMIT - 4)}`)
expectWarning()
})

it('replaces forbidden characters with slashes', () => {
expect(buildTag('env', 'b#r')).toBe('env:b_r')
expectWarning()
})

function expectWarning() {
expect(displaySpy).toHaveBeenCalledOnceWith("env value doesn't meet tag requirements and will be sanitized")
}
})
36 changes: 36 additions & 0 deletions packages/core/src/domain/configuration/tags.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
import { display } from '../../tools/display'
import { InitConfiguration } from './configuration'

export const TAG_SIZE_LIMIT = 200

export function buildTags(configuration: InitConfiguration): string[] {
const { env, service, version } = configuration
const tags = []

if (env) {
tags.push(buildTag('env', env))
}
if (service) {
tags.push(buildTag('service', service))
}
if (version) {
tags.push(buildTag('version', version))
}

return tags
}

export function buildTag(key: string, rawValue: string) {
// See https://docs.datadoghq.com/getting_started/tagging/#defining-tags for tags syntax.
const valueSizeLimit = TAG_SIZE_LIMIT - key.length - 1
const sanitizedValue = rawValue
.toLowerCase()
.slice(0, valueSizeLimit)
.replace(/[^a-z0-9_:./-]/g, '_')

if (sanitizedValue !== rawValue) {
display.warn(`${key} value doesn't meet tag requirements and will be sanitized`)
}

return `${key}:${sanitizedValue}`
}
Original file line number Diff line number Diff line change
Expand Up @@ -131,9 +131,9 @@ describe('transportConfiguration', () => {

describe('tags', () => {
it('should be encoded', () => {
const configuration = computeTransportConfiguration({ clientToken, service: 'bar+foo' }, buildEnv)
const configuration = computeTransportConfiguration({ clientToken, service: 'bar:foo' }, buildEnv)
expect(configuration.rumEndpointBuilder.build()).toContain(
`ddtags=sdk_version%3Asome_version%2Cservice%3Abar%2Bfoo`
`ddtags=sdk_version%3Asome_version%2Cservice%3Abar%3Afoo`
)
})
})
Expand Down
24 changes: 15 additions & 9 deletions packages/core/src/domain/configuration/transportConfiguration.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { BuildEnv, BuildMode } from '../../boot/init'
import { objectValues } from '../../tools/utils'
import { InitConfiguration } from './configuration'
import { createEndpointBuilder, INTAKE_SITE_US, EndpointBuilder } from './endpointBuilder'
import { buildTags } from './tags'

export interface TransportConfiguration {
logsEndpointBuilder: EndpointBuilder
Expand All @@ -24,10 +25,12 @@ export function computeTransportConfiguration(
initConfiguration: InitConfiguration,
buildEnv: BuildEnv
): TransportConfiguration {
const endpointBuilders = computeEndpointBuilders(initConfiguration, buildEnv)
const tags = buildTags(initConfiguration)

const endpointBuilders = computeEndpointBuilders(initConfiguration, buildEnv, tags)
const intakeEndpoints = objectValues(endpointBuilders).map((builder) => builder.buildIntakeUrl())

const replicaConfiguration = computeReplicaConfiguration(initConfiguration, buildEnv, intakeEndpoints)
const replicaConfiguration = computeReplicaConfiguration(initConfiguration, buildEnv, intakeEndpoints, tags)

return {
isIntakeUrl: (url) => intakeEndpoints.some((intakeEndpoint) => url.indexOf(intakeEndpoint) === 0),
Expand All @@ -36,7 +39,7 @@ export function computeTransportConfiguration(
}
}

function computeEndpointBuilders(initConfiguration: InitConfiguration, buildEnv: BuildEnv) {
function computeEndpointBuilders(initConfiguration: InitConfiguration, buildEnv: BuildEnv, tags: string[]) {
if (buildEnv.buildMode === BuildMode.E2E_TEST) {
const e2eEndpointBuilder = (placeholder: string) => ({
build: () => placeholder,
Expand All @@ -52,9 +55,9 @@ function computeEndpointBuilders(initConfiguration: InitConfiguration, buildEnv:
}

const endpointBuilders = {
logsEndpointBuilder: createEndpointBuilder(initConfiguration, buildEnv, 'logs'),
rumEndpointBuilder: createEndpointBuilder(initConfiguration, buildEnv, 'rum'),
sessionReplayEndpointBuilder: createEndpointBuilder(initConfiguration, buildEnv, 'sessionReplay'),
logsEndpointBuilder: createEndpointBuilder(initConfiguration, buildEnv, 'logs', tags),
rumEndpointBuilder: createEndpointBuilder(initConfiguration, buildEnv, 'rum', tags),
sessionReplayEndpointBuilder: createEndpointBuilder(initConfiguration, buildEnv, 'sessionReplay', tags),
}

if (initConfiguration.internalMonitoringApiKey) {
Expand All @@ -64,6 +67,7 @@ function computeEndpointBuilders(initConfiguration: InitConfiguration, buildEnv:
{ ...initConfiguration, clientToken: initConfiguration.internalMonitoringApiKey },
buildEnv,
'logs',
tags,
'browser-agent-internal-monitoring'
),
}
Expand All @@ -75,7 +79,8 @@ function computeEndpointBuilders(initConfiguration: InitConfiguration, buildEnv:
function computeReplicaConfiguration(
initConfiguration: InitConfiguration,
buildEnv: BuildEnv,
intakeEndpoints: string[]
intakeEndpoints: string[],
tags: string[]
): ReplicaConfiguration | undefined {
if (buildEnv.buildMode !== BuildMode.STAGING || initConfiguration.replica === undefined) {
return
Expand All @@ -89,12 +94,13 @@ function computeReplicaConfiguration(
}

const replicaEndpointBuilders = {
logsEndpointBuilder: createEndpointBuilder(replicaConfiguration, buildEnv, 'logs'),
rumEndpointBuilder: createEndpointBuilder(replicaConfiguration, buildEnv, 'rum'),
logsEndpointBuilder: createEndpointBuilder(replicaConfiguration, buildEnv, 'logs', tags),
rumEndpointBuilder: createEndpointBuilder(replicaConfiguration, buildEnv, 'rum', tags),
internalMonitoringEndpointBuilder: createEndpointBuilder(
replicaConfiguration,
buildEnv,
'logs',
tags,
'browser-agent-internal-monitoring'
),
}
Expand Down
2 changes: 1 addition & 1 deletion packages/core/src/transport/httpRequest.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ describe('httpRequest intake parameters', () => {

beforeEach(() => {
server = sinon.fakeServer.create()
endpointBuilder = createEndpointBuilder({ clientToken }, buildEnv, 'logs')
endpointBuilder = createEndpointBuilder({ clientToken }, buildEnv, 'logs', [])
request = new HttpRequest(endpointBuilder, BATCH_BYTES_LIMIT)
})

Expand Down
6 changes: 3 additions & 3 deletions packages/logs/src/boot/startLogs.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ const baseConfiguration: Partial<LogsConfiguration> = {
...DEFAULT_CONFIGURATION,
logsEndpointBuilder: stubEndpointBuilder('https://localhost/v1/input/log'),
maxBatchSize: 1,
service: 'Service',
service: 'service',
}
const internalMonitoring = { setExternalContextProvider: () => undefined }

Expand Down Expand Up @@ -105,7 +105,7 @@ describe('logs', () => {
date: FAKE_DATE as TimeStamp,
foo: 'bar',
message: 'message',
service: 'Service',
service: 'service',
session_id: SESSION_ID,
status: StatusType.warn,
view: {
Expand Down Expand Up @@ -234,7 +234,7 @@ describe('logs', () => {
expect(assembledMessage).toEqual({
foo: 'from-current-context',
message: DEFAULT_MESSAGE.message,
service: 'Service',
service: 'service',
session_id: SESSION_ID,
status: DEFAULT_MESSAGE.status,
view: { url: 'http://from-rum-context.com', id: 'view-id' },
Expand Down
6 changes: 3 additions & 3 deletions test/e2e/scenario/rum/tracing.scenario.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { flushEvents } from '../../lib/helpers/flushEvents'

describe('tracing', () => {
createTest('trace xhr')
.withRum({ service: 'Service', allowedTracingOrigins: ['LOCATION_ORIGIN'] })
.withRum({ service: 'service', allowedTracingOrigins: ['LOCATION_ORIGIN'] })
.run(async ({ serverEvents }) => {
const rawHeaders = await sendXhr(`/headers`, [
['x-foo', 'bar'],
Expand All @@ -16,7 +16,7 @@ describe('tracing', () => {
})

createTest('trace fetch')
.withRum({ service: 'Service', allowedTracingOrigins: ['LOCATION_ORIGIN'] })
.withRum({ service: 'service', allowedTracingOrigins: ['LOCATION_ORIGIN'] })
.run(async ({ serverEvents }) => {
const rawHeaders = await browserExecuteAsync<string | Error>((done) => {
window
Expand All @@ -39,7 +39,7 @@ describe('tracing', () => {
})

createTest('trace fetch with Request argument')
.withRum({ service: 'Service', allowedTracingOrigins: ['LOCATION_ORIGIN'] })
.withRum({ service: 'service', allowedTracingOrigins: ['LOCATION_ORIGIN'] })
.run(async ({ serverEvents }) => {
const rawHeaders = await browserExecuteAsync<string | Error>((done) => {
window
Expand Down

0 comments on commit dfa4033

Please sign in to comment.