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

Enabled analytics logging to opensearch #9145

Draft
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

ashish-aesthisia
Copy link
Contributor

@ashish-aesthisia ashish-aesthisia commented Oct 25, 2023

Summary

🤖 Generated by Copilot at c6cf091

This pull request implements analytics collection for the task server. It adds a new file collect-analytics.ts that handles the data processing and storage.

References

closes #insert number here

Explanation

🤖 Generated by Copilot at c6cf091

  • Implement analytics collection feature for the task server (packages/taskserver/src/collect-analytics.ts)
  • Declare variables to store timestamp, analytics data, and collected data (link)
  • Filter out and log the data that has not been collected yet, and update timestamp (link)
  • Collect and push user data from instance and channel to collected data array (link, link)
  • Collect and push instance data to collected data array (link)

🤖 Generated by Copilot at c6cf091

We're sailing on the task server, aye, aye, aye
We're logging all the analytics, aye, aye, aye
We use collect-analytics.ts to store and sort
And we pull the data when we reach the port

QA Steps

List any additional steps required to QA the changes of this PR, as well as any supplemental images or videos.

Checklist

  • If this PR is still a WIP, convert to a draft
  • When this PR is ready, mark it as "Ready for review"
  • ensure all checks pass
  • Changes have been manually QA'd
  • Changes reviewed by at least 2 approved reviewers

@@ -38,6 +38,9 @@ const DEFAULT_INTERVAL_SECONDS = 1800
const configInterval = parseInt(config.taskserver.processInterval)
const interval = (configInterval || DEFAULT_INTERVAL_SECONDS) * 1000

let lastTimestamp: string
let analyticsData: any[] = []
const collectedData: any[] = []
Copy link
Contributor

Choose a reason for hiding this comment

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

Will collectedData continue to grow in memory infinitely? I think it will also repeat a lot of data because of that. Unless you're filtering on the Opensource ingest side.

const currentTimestamp = new Date().toISOString()

if (lastTimestamp) {
analyticsData = analyticsData.filter((data) => {
Copy link
Contributor

@DanielBelmes DanielBelmes Oct 30, 2023

Choose a reason for hiding this comment

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

I don't believe there is any data in analyticsData. Plus it is not being used.

Copy link
Contributor

@DanielBelmes DanielBelmes left a comment

Choose a reason for hiding this comment

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

I think there's some changes needed to ensure we're not growing in memory. Plus I think the analyticsData section isn't functioning in its current state.

@HexaField HexaField marked this pull request as draft November 11, 2023 00:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants