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

feat: save rum-traffic as metrics to s3 #631

Closed
wants to merge 8 commits into from
Closed

feat: save rum-traffic as metrics to s3 #631

wants to merge 8 commits into from

Conversation

rpapani
Copy link
Contributor

@rpapani rpapani commented Feb 5, 2025

For calculating the projected traffic lost/value, we need organic traffic from RUM for the given page. Rather than each opportunity crunching the RUM data separately, adding this rum-traffic audit, which will save the rum-traffic data to importer metrics.
So, the opportunities can obtain the rum-traffic data as

import { getStoredMetrics } from '@adobe/spacecat-shared-utils';

const rumTrafficData = await getStoredMetrics(
    { source: 'rum', metric: 'rum-traffic', siteId },
    context,
  );
const pageOrganicTraffic = rumTrafficData['<url>']?.earned || 0;

Copy link

github-actions bot commented Feb 5, 2025

This PR will trigger a minor release when merged.

@@ -58,6 +59,7 @@ const HANDLERS = {
'structured-data': structuredData,
'forms-opportunities': formsOpportunities,
'site-detection': siteDetection,
'rum-traffic': rumTraffic,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

any preference rum-traffic vs rum-metrics ?

Copy link
Contributor

Choose a reason for hiding this comment

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

looks like it's only about traffic data, not other metrics. Would keep rum-traffic in that case

@ekremney ekremney requested review from solaris007 and iuliag February 6, 2025 08:59
@ekremney
Copy link
Member

ekremney commented Feb 6, 2025

@iuliag please review this one: possible overlap with import worker

Copy link
Member

@solaris007 solaris007 left a comment

Choose a reason for hiding this comment

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

  • this should be an import, not an audit. please move to import worker
  • please make sure you add relevant configurations to global config / jobs dispatcher
  • getStoredMetrics should not be part of utils

src/rum-traffic/rum-traffic.js Show resolved Hide resolved
@dzehnder dzehnder self-requested a review February 6, 2025 09:21
Copy link
Contributor

@iuliag iuliag left a comment

Choose a reason for hiding this comment

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

This should be an import of type page-organic-traffic with source rum and destination default (meaning S3).

During the workshop, there were 2 discussions leading to the decision to have the metrics collection running independent of the auditing:

  • ingestor service for unified metrics collection so that auto-identify and auto-suggest have all kinds of metrics available without duplicating data collection implementation and increasing COGS (see diagram in Experience Success Studio -> ingestor service tab)
  • timeline for metrics collection to cover needs of the success report (whiteboarding from workshop)

@rpapani
Copy link
Contributor Author

rpapani commented Feb 12, 2025

@iuliag

This should be an import of type page-organic-traffic with source rum and destination default (meaning S3).

sure, will move the logic to import worker

ingestor service for unified metrics collection so that auto-identify and auto-suggest have all kinds of metrics available without duplicating data collection implementation and increasing COGS

is this ingestor service available already? or the future service that we'll implement post GA? If I implement this page-organic-traffic in import worker, will this be ported to ingestor service later?

timeline for metrics collection to cover needs of the success report
I'm using 30 days as a window for collecting this data. If we make this timeline configurable, then we'll have to add that information in the metrics as well to inform the consumers about the timeline.

@rpapani rpapani closed this Feb 12, 2025
@iuliag
Copy link
Contributor

iuliag commented Feb 12, 2025

is this ingestor service available already? or the future service that we'll implement post GA? If I implement this page-organic-traffic in import worker, will this be ported to ingestor service later?

it's not available already. we'll implement it post-GA starting from what we have in the import worker

I'm using 30 days as a window for collecting this data. If we make this timeline configurable, then we'll have to add that information in the metrics as well to inform the consumers about the timeline.

we have a time property for each metric entry https://github.com/adobe/spacecat-api-service/blob/main/docs/openapi/schemas.yaml#L1403-L1432 and you could also add either a granularity field (monthly, weekly etc.) or the time interval.

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.

5 participants