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

Allow custom metrics endpoint #1037

Merged
merged 4 commits into from
Mar 5, 2024
Merged

Allow custom metrics endpoint #1037

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
bf2f2cf
allow custom metrics endpoint
danieljackins Feb 13, 2024
2f80324
changeset
danieljackins Feb 13, 2024
2984646
set host with load options, respect protocol for metrics
danieljackins Mar 5, 2024
9baea6d
narrow protocol type
danieljackins Mar 5, 2024
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
5 changes: 5 additions & 0 deletions .changeset/few-pets-tell.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@segment/analytics-next': minor
---

Allow custom metrics endpoint
9 changes: 8 additions & 1 deletion packages/browser/src/browser/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -353,7 +353,14 @@ async function loadAnalytics(
const plugins = settings.plugins ?? []

const classicIntegrations = settings.classicIntegrations ?? []
Stats.initRemoteMetrics(legacySettings.metrics)

if (options.metricsEndpoint && legacySettings.metrics) {
legacySettings.metrics.host = options.metricsEndpoint
}

Stats.initRemoteMetrics(
legacySettings.metrics ?? { host: options.metricsEndpoint }
)
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if Stats.initRemoteMetrics receives { host: undefined } ? Given that we were always sending the legacySettings.metrics object which looks like could be either full or undefined.

I'm also curious if we need this coalescing here as just above we're already overriding it.


// needs to be flushed before plugins are registered
flushPreBuffer(analytics, preInitBuffer)
Expand Down
4 changes: 4 additions & 0 deletions packages/browser/src/core/analytics/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,10 @@ export interface InitOptions {
plan?: Plan
retryQueue?: boolean
obfuscate?: boolean
/**
* Allows you to change the metrics endpoint used
*/
metricsEndpoint?: string
Copy link
Contributor

@silesky silesky Feb 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Can you provide some background about the impetus / use case for a new setting (maybe link to the issue / description is OK)? (I saw an issue around this, but I figured we would just default to the overridden apiHost / protocol.)

i.e.

integrations: {
        'Segment.io': {
          // assume https://MY-CUSTOM-API-PROXY.com/v1/m  proxies to
          // https://api.segment.io/v1/m
          apiHost: 'MY-CUSTOM-API-PROXY.com/v1',
          protocol: 'https' // optional
        }

(forgive me if I am missing something silly)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No that makes sense! Updated to default to that value and to respect protocol if set

/**
* This callback allows you to update/mutate CDN Settings.
* This is called directly after settings are fetched from the CDN.
Expand Down
Loading