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

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

Merged
merged 2 commits into from
Jan 11, 2021
Merged

Conversation

bcaudan
Copy link
Contributor

@bcaudan bcaudan commented Jan 8, 2021

Motivation

Only alternate intake will be available for new data centers.
us3 alternate intake is now available.

Changes

Temporary strategy to force alternate intake for us3.
When new intake will be available for gov, we should only allow classic intake for us and eu.

Testing

Automated tests + us3 test


I have gone over the contributing documentation.

@bcaudan bcaudan requested a review from a team as a code owner January 8, 2021 15:29
Copy link
Contributor

@webNeat webNeat left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@BenoitZugmeyer BenoitZugmeyer left a comment

Choose a reason for hiding this comment

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

It seems fine, but I want to point to a related commit I had to do for rum-recorder: 00cd89e

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

@@ -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.

@bcaudan bcaudan merged commit e989a5d into master Jan 11, 2021
@bcaudan bcaudan deleted the bcaudan/azure-intake branch January 11, 2021 08:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants