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.

Sign up for GitHub

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

Jump to bottom

[RUMF-810] force alternate intake for us3 #677

Merged
merged 2 commits into from
Jan 11, 2021
Merged

[RUMF-810] force alternate intake for us3 #677

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5fb7728
[RUMF-810] force alternate intake for us3
bcaudan Jan 8, 2021
f19da98
Merge branch 'master' into bcaudan/azure-intake
bcaudan Jan 8, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 15 additions & 5 deletions packages/core/src/domain/configuration.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,17 +133,27 @@ describe('configuration', () => {
})

it('should handle sites with subdomains and classic intake', () => {
const configuration = buildConfiguration({ clientToken, site: 'us3.datadoghq.com' }, usEnv)
expect(configuration.isIntakeUrl('https://rum-http-intake.logs.us3.datadoghq.com/v1/input/xxx')).toBe(true)
expect(configuration.isIntakeUrl('https://browser-http-intake.logs.us3.datadoghq.com/v1/input/xxx')).toBe(true)
expect(configuration.isIntakeUrl('https://public-trace-http-intake.logs.us3.datadoghq.com/v1/input/xxx')).toBe(
const configuration = buildConfiguration({ clientToken, site: 'foo.datadoghq.com' }, usEnv)
expect(configuration.isIntakeUrl('https://rum-http-intake.logs.foo.datadoghq.com/v1/input/xxx')).toBe(true)
expect(configuration.isIntakeUrl('https://browser-http-intake.logs.foo.datadoghq.com/v1/input/xxx')).toBe(true)
expect(configuration.isIntakeUrl('https://public-trace-http-intake.logs.foo.datadoghq.com/v1/input/xxx')).toBe(
true
)
})

it('should handle sites with subdomains and alternate intake', () => {
const configuration = buildConfiguration(
{ clientToken, site: 'us3.datadoghq.com', useAlternateIntakeDomains: true },
{ clientToken, site: 'foo.datadoghq.com', useAlternateIntakeDomains: true },
usEnv
)
expect(configuration.isIntakeUrl('https://rum.browser-intake-foo-datadoghq.com/v1/input/xxx')).toBe(true)
expect(configuration.isIntakeUrl('https://logs.browser-intake-foo-datadoghq.com/v1/input/xxx')).toBe(true)
expect(configuration.isIntakeUrl('https://trace.browser-intake-foo-datadoghq.com/v1/input/xxx')).toBe(true)
})

it('should force alternate intake for us3', () => {
const configuration = buildConfiguration(
{ clientToken, site: 'us3.datadoghq.com', useAlternateIntakeDomains: false },
usEnv
)
expect(configuration.isIntakeUrl('https://rum.browser-intake-us3-datadoghq.com/v1/input/xxx')).toBe(true)
Expand Down
7 changes: 6 additions & 1 deletion packages/core/src/domain/configuration.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ export function buildConfiguration(userConfiguration: UserConfiguration, buildEn
? userConfiguration.enableExperimentalFeatures
: []

const intakeType: IntakeType = userConfiguration.useAlternateIntakeDomains ? 'alternate' : 'classic'
const intakeType: IntakeType = getIntakeType(transportConfiguration.site, userConfiguration)
const intakeUrls = getIntakeUrls(intakeType, transportConfiguration, userConfiguration.replica !== undefined)
const configuration: Configuration = {
beforeSend: userConfiguration.beforeSend,
Expand Down Expand Up @@ -256,6 +256,11 @@ function getHost(intakeType: IntakeType, endpointType: EndpointType, site: strin
return `${endpoint}.browser-intake-${suffix}`
}

function getIntakeType(site: string, userConfiguration: UserConfiguration) {
// TODO when new intake will be available for gov, only allow classic intake for us and eu
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't we do this already? something like:

if (!userConfiguration.useAlternateIntakeDomains && (site === 'datadoghq.com' || site === 'datadoghq.eu')) {
  return 'classic'
}
return 'alternate'

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this would not allow classic intake for gov or am I missing something?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about the specific rules we should apply, but it seems that new datacenters won't have classic intake anymore. Thus, I think that it would be better if we had a list of sites that support classic intakes (since it is not likely to change in the future), instead of a list of sites that don't support classic intakes (since this list is likely to grow when new datacenters are added).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is what I have in mind too but I'd prefer to not break rum on dd gov app with this PR.
We could implement this logic now and define gov as supporting classic but we would still need an extra PR to remove it.
no strong opinion about one way or the other.

It seems that our intake host logic gets more complex, and I wonder if we shouldn't centralize/colocate this logic.

agree, we could wait for the PR for gov since this part should not change too much in the mean time and it would avoid to deal with conflicts for now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine either way. We should not take conflicts into account: my PR doesn't change the same part of configuration.ts so we shouldn't have any conflict. We can think again how to improve on this once everything gets merged.

return userConfiguration.useAlternateIntakeDomains || site === 'us3.datadoghq.com' ? 'alternate' : 'classic'
}

function getIntakeUrls(intakeType: IntakeType, conf: TransportConfiguration, withReplica: boolean) {
if (conf.proxyHost) {
return [`https://${conf.proxyHost}/v1/input/`]
Expand Down