[browserstack-service] Add Funnel Data instrumentation [v8] #12392

sriteja777 · 2024-03-01T15:39:53Z

Proposed changes

Send instrumentation details at start and end of build
Refactor code to use a single listener for all frameworks test details reporting.

Types of changes

Polish (an improvement for an existing feature)
Bugfix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation update (improvements to the project's docs)
Specification changes (updates to WebDriver command specifications)
Internal updates (everything related to internal scripts, governance documentation and CI files)

Checklist

I have read the CONTRIBUTING doc
I have added tests that prove my fix is effective or that my feature works
I have added the necessary documentation (if appropriate)
I have added proper type definitions for new commands (if appropriate)

Backport Request

//: # (The current main branch is the development branch for WebdriverIO v9. If your change should be released to the current major version of WebdriverIO (v8), please raise another PR with the same changes against the v8 branch.)

This change is solely for v9 and doesn't need to be back-ported
Back-ported PR at #XXXXX

Further comments

Reviewers: @webdriverio/project-committers

christian-bromann

Thanks for the PR. I would generally advise not to wrap everything within a try/catch.

packages/wdio-browserstack-service/src/data-store.ts

packages/wdio-browserstack-service/src/exitHandler.ts

packages/wdio-browserstack-service/src/instrumentation/funnelInstrumentation.ts

packages/wdio-browserstack-service/src/testOps/listener.ts

christian-bromann · 2024-03-01T20:16:54Z

packages/wdio-browserstack-service/src/testOps/listener.ts

+        try {
+            this.hookStartedStats.triggered()
+            this.sendBatchEvents(this.getEventForHook('HookRunStarted', hookData))


Why is this wrapped in a try/catch block?

The sendBatchEvents method calls add method of RequestQueueHandler which can throw exception

currently when build start not yet completed.

This try catch is added to mainly get the proper instrumentation on how many events, we were able to send properly and how many failed. That's why this try catch is needed to catch the instrumentation properly, we are re-throwing the exception from here after catching.

christian-bromann · 2024-03-01T20:17:00Z

packages/wdio-browserstack-service/src/testOps/listener.ts

+        try {
+            this.hookFinishedStats.triggered(hookData.result)
+            this.sendBatchEvents(this.getEventForHook('HookRunFinished', hookData))


The sendBatchEvents method calls add method of RequestQueueHandler which can throw exception

currently when build start not yet completed.

This try catch is added to mainly get the proper instrumentation on how many events, we were able to send properly and how many failed. That's why this try catch is needed to catch the instrumentation properly, we are re-throwing the exception from here after catching.

christian-bromann · 2024-03-01T20:18:22Z

packages/wdio-browserstack-service/src/testOps/requestUtils.ts

+        }).json()
+        BStackLogger.debug(`[${logTag}] Success response: ${JSON.stringify(data)}`)
+    } catch (error) {
+        BStackLogger.debug(`[${logTag}] Failed. Error: ${error}`)


Shouldn't we return an error object here too similar to line 23?

Actually this method should throw error for any reason it is not able to make the request, forgot to update it before. Have updated it now to throw errors

packages/wdio-browserstack-service/src/testOps/usageStats.ts

sriteja777 · 2024-03-04T19:32:03Z

Hey @christian-bromann, thanks for the quick review of the PR and sorry for my delay in responding. Have replied to all comments and made changes wherever necessary. Please re-review the PR

christian-bromann

Two last comments. Can we raise a similar PR to main and optionally v7 if you like to backport this.

packages/wdio-browserstack-service/src/config.ts

packages/wdio-browserstack-service/src/request-handler.ts

Co-authored-by: Christian Bromann <git@bromann.dev>

…o add_observability_stats

sriteja777 · 2024-03-05T18:33:09Z

Hey @christian-bromann, have resolved the comments. Also the failing tests doesn't seem related to the changes.

For v7, I am creating the PR will share for review in some while.
For v9, my current local setup is breaking with v9. Need to check more on it. It may take some time to create a v9 PR for this.

christian-bromann · 2024-03-05T18:54:12Z

For v9, my cuurent local setup is breaking with v9.

Let me know if I can help to resolve the issue. I recommend checking out v9 in a separate directory since we switched to pnpm which resolves dependencies very differently.

That said, I wanted to make sure that we have all 3 PRs ready to go before merging.

sriteja777 · 2024-03-05T19:23:20Z

v9 is working for me locally now. I was trying out v9 branch day before yesterday, but faced some typescript errors when ran the setup. Not sure what merged in main now, but with latest pull, currently it is working for me 🎉 . Will try to create the v9 PR for this.

@christian-bromann It might take some time for me (possibly tomorrow) to create a v9 PR as I need to port the changes and test them. Possible for merging this PR for now? Will share the v7 and v9 PRs as soon as they are ready.

christian-bromann · 2024-03-05T19:28:23Z

I am happy to merge once we have the PR for main branch to ensure we don't diverge between both future supported versions.

sriteja777 · 2024-03-05T19:32:25Z

Sure @christian-bromann, I will try to create rest of the PRs asap. Thanks!

sriteja777 · 2024-03-06T18:01:29Z

Hey @christian-bromann, have created PRs for main and v7. Please have a look

christian-bromann

LGTM 👍

packages/wdio-browserstack-service/tests/request-handler.test.ts

sriteja777 added 12 commits February 23, 2024 02:43

add TestObservability usage stats

9f63d5f

eslint fixes

351da2c

eslint fixes

cb71a28

send funnel data from cleanup

ddf474a

add test errors and refactor workers data

67b236a

remove test errors code

847f9ce

review comments fixes

e2e4f26

review comment fixes

8464ac5

removed events from log

eb592d6

add and fix tests

6d15a1f

fix tests

8c3ef82

refactor

876ae3c

christian-bromann requested changes Mar 1, 2024

View reviewed changes

sriteja777 and others added 2 commits March 5, 2024 00:36

refactor code and fix tests

a498f3c

Merge branch 'v8' into add_observability_stats

a27b982

sriteja777 requested a review from christian-bromann March 4, 2024 19:32

sriteja777 added 3 commits March 5, 2024 01:13

refactor code and add comments

b6af4bf

store env in constants.ts

e82ce79

fix sending funnel data from cleanup

ac34c58

christian-bromann reviewed Mar 5, 2024

View reviewed changes

packages/wdio-browserstack-service/src/config.ts Outdated Show resolved Hide resolved

packages/wdio-browserstack-service/src/request-handler.ts Outdated Show resolved Hide resolved

sriteja777 and others added 4 commits March 5, 2024 23:23

Update packages/wdio-browserstack-service/src/config.ts

e872400

Co-authored-by: Christian Bromann <git@bromann.dev>

Update packages/wdio-browserstack-service/src/request-handler.ts

eefb18c

Co-authored-by: Christian Bromann <git@bromann.dev>

empty commit to trigger tests

2d9a4e5

Merge remote-tracking branch 'sriteja777/add_observability_stats' int…

a655db6

…o add_observability_stats

sriteja777 requested a review from christian-bromann March 5, 2024 18:30

christian-bromann added the PR: New Feature 🚀 PRs that contain new features label Mar 5, 2024

fix empty request and send funnel data from onPrepare hook

306d99d

sriteja777 mentioned this pull request Mar 6, 2024

[browserstack-service] Add Funnel Data instrumentation [v9] #12442

Merged

14 tasks

sriteja777 changed the title ~~[browserstack-service] Add Funnel Data instrumentation~~ [browserstack-service] Add Funnel Data instrumentation [v8] Mar 6, 2024

Merge branch 'v8' into add_observability_stats

b504e8d

christian-bromann approved these changes Mar 6, 2024

View reviewed changes

packages/wdio-browserstack-service/tests/request-handler.test.ts Outdated Show resolved Hide resolved

Update packages/wdio-browserstack-service/tests/request-handler.test.ts

a8d79f9

christian-bromann merged commit 915f780 into webdriverio:v8 Mar 6, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[browserstack-service] Add Funnel Data instrumentation [v8] #12392

[browserstack-service] Add Funnel Data instrumentation [v8] #12392

sriteja777 commented Mar 1, 2024 •

edited

Loading

christian-bromann left a comment

christian-bromann Mar 1, 2024

sriteja777 Mar 4, 2024

christian-bromann Mar 1, 2024

sriteja777 Mar 4, 2024

christian-bromann Mar 1, 2024

sriteja777 Mar 4, 2024

sriteja777 commented Mar 4, 2024

christian-bromann left a comment

sriteja777 commented Mar 5, 2024 •

edited

Loading

christian-bromann commented Mar 5, 2024 •

edited

Loading

sriteja777 commented Mar 5, 2024

christian-bromann commented Mar 5, 2024

sriteja777 commented Mar 5, 2024 •

edited

Loading

sriteja777 commented Mar 6, 2024

christian-bromann left a comment

[browserstack-service] Add Funnel Data instrumentation [v8] #12392

[browserstack-service] Add Funnel Data instrumentation [v8] #12392

Conversation

sriteja777 commented Mar 1, 2024 • edited Loading

Proposed changes

Types of changes

Checklist

Backport Request

Further comments

Reviewers: @webdriverio/project-committers

christian-bromann left a comment

Choose a reason for hiding this comment

christian-bromann Mar 1, 2024

Choose a reason for hiding this comment

sriteja777 Mar 4, 2024

Choose a reason for hiding this comment

christian-bromann Mar 1, 2024

Choose a reason for hiding this comment

sriteja777 Mar 4, 2024

Choose a reason for hiding this comment

christian-bromann Mar 1, 2024

Choose a reason for hiding this comment

sriteja777 Mar 4, 2024

Choose a reason for hiding this comment

sriteja777 commented Mar 4, 2024

christian-bromann left a comment

Choose a reason for hiding this comment

sriteja777 commented Mar 5, 2024 • edited Loading

christian-bromann commented Mar 5, 2024 • edited Loading

sriteja777 commented Mar 5, 2024

christian-bromann commented Mar 5, 2024

sriteja777 commented Mar 5, 2024 • edited Loading

sriteja777 commented Mar 6, 2024

christian-bromann left a comment

Choose a reason for hiding this comment

sriteja777 commented Mar 1, 2024 •

edited

Loading

sriteja777 commented Mar 5, 2024 •

edited

Loading

christian-bromann commented Mar 5, 2024 •

edited

Loading

sriteja777 commented Mar 5, 2024 •

edited

Loading