fix: check supportedEntryTypes when starting observer

[kyungeunni]

fixes #1245

This PR checks PerformanceObserver.supportedEntryTypes before starting an observer for a certain entryType so that it doesn't print a warning on Firefox browser and doesn't trigger errors on Safari when it starts observer for unsupported types.

Before the change on Firefox (latest version):

After the change on Firefox (local build of this PR):

Note

List of supported entry types:

• Chrome (102.0):
  ['element', 'event', 'first-input', 'largest-contentful-paint', 'layout-shift', 'longtask', 'mark', 'measure', 'navigation', 'paint', 'resource']
• Firefox(101.0.1): [ "event", "first-input", "mark", "measure", "navigation", "paint", "resource"]
• Safari(15.5):
  ["mark", "measure", "navigation", "paint", "resource"]

[kyungeunni]

I've removed try-catch based on the description in the comment, if folks think it's too risky, I will put it back.

[devcorpio]

I would remove the comment but I wouldn't remove the try-catch.

• Main reason: It's a browser API, I would treat it as an external dependency and I would follow a defensive approach.
• The spec mention "error" and "exception" multiple times. Theoretically the only that can affect the agent (due to the way we are using the API) is the one about the invalid entry type, theoretically... 😄
• There are a ton of browsers (and versions) out there that we are supporting on 5.x versions. Besides, to see bugs like the one commented on this issue: https://github.com/elastic/sdh-kibana/issues/2547#issuecomment-1029054684 it's not completely ruled out.

We can add a comment inside the catch if you think that might be helpful for the future us and future contributors. Something like this

WDYT?

[kyungeunni]

Thanks Alberto, it all makes 💯 sense!! I've put back try-catch and added some comment inside catch 🙏

[apmmachine]

📦 Bundlesize report

Filename	Size(bundled)	Size(gzip)	Diff(gzip)
elastic-apm-opentracing.umd.min.js	66.0 KiB	21.0 KiB	⚠️ 7 Bytes
elastic-apm-rum.umd.min.js	59.9 KiB	19.5 KiB	⚠️ 7 Bytes

[apmmachine]

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

• Start Time: 2022-06-16T17:20:44.843+0000

• Duration: 76 min 51 sec

Test stats 🧪

Test	Results
Failed	0
Passed	4740
Skipped	64
Total	4804

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

• /test : Re-trigger the build.

• run benchmark tests : Run the benchmark test.

• run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

[devcorpio]

You can use the isPerfTypeSupported util instead.

[kyungeunni]

Oh wow, I didn't know it existed!! 🌟

[devcorpio]

Just thinking out loud:

supportedEntryTypes became available later than observe

For instance:

If we stop reporting based on supportedEntryTypes we might start missing data. Okay, maybe in the case of Chrome and Firefox it's not a big deal since versions before 73 and 68 are old. Besides, they are evergreen browsers.

The case of Safari might be different. We support older versions such the 12. What is more, Safari is not an evergreen browser. 😭 As you can see here and here

It seems that we might be adding a breaking change discreetly.

Questions:

• Does the firefox warning has more importance than the versioning thingy?
• Is firefox the only browser reporting the warnings?

If firefox is the only one complaining and you both believe that what I commented above might be a problem, a possible proposal might be:

Step 1:

We are in 5.x version, we don't want new releases to cause unexpected regressions. What about checking if the user agent is firefox for the following entry types: largest-contentul-paint, layout-shift, and long-task. I know that checking user-agents is tricky. But I wanted to share this at least.

Step 2:

Once we go to 6.x version, there is where we can do breaking changes. For instance, "We support the following browsers, etc, etc". And then we can remove the logic added in Step 1

--

I considered that at least was important to bring out this concern

What are your thoughts, @kyungeunni @vigneshshanmugam?

[kyungeunni]

This is a great point, thanks for bringing this up.

As you mentioned, most of the browsers shipped supportedEntryTypes later than observe so I can see this change will introduce regression as fewer metrics will be observed, even though the entryType is supported for the browsers.

I appreciate your suggestion on Step1, although would it be fair to assume the possibility each version of FF could have a different set of supported entry types? (e.g. let's say version 57 supports 3 entry types, version 60 supports 5 so not only largest-contentul-paint, layout-shift, and long-task would log the warning messages but other unsupported ones too). Also, there are uninvestigated browsers too that could possibly log the warning messages as same as FF does. That being said, it seems like we can't be sure to cover all the cases with this approach, It'd look like we didn't really fix this bug as we didn't cover all the cases.

I'd love to chat about plans for supported browsers for 6.x and plan this change accordingly. Meanwhile, could we communicate with users that this is how Firefox-specific behavior and if we try to mute the log, it would introduce regression as we will capture fewer metrics? If it's not a critical bug from the user's perspective, that is.

[devcorpio]

I agree with you here. Likely, the most sensible thing we can do is not to mute the log since a clean solution without supportedEntryTypes seems a bit tricky and probably it's not worth the effort. So I also think that we can wait until 6.x to retackle this. I would like to know @vigneshshanmugam thoughts before closing the PR though.

The issue related to this PR probably will end up having the breaking-change label. And also we will need inform about our decision to the support engineer that brought out this issue.

[vigneshshanmugam]

I would like us to think this in a different way. The main types we use for Observer are largest-contentful-paint, long-tasks, paint, input-delay -

apm-agent-rum-js/packages/rum-core/src/performance-monitoring/transaction-service.js

Lines 166 to 169 in 96c7e69

	this.recorder.start(LARGEST_CONTENTFUL_PAINT)
	this.recorder.start(PAINT)
	this.recorder.start(FIRST_INPUT)
	this.recorder.start(LAYOUT_SHIFT)

.

I believe none of those metrics would be supported by the older browsers such as Safari 11. If we can confirm that, then we should be good to do just doing a check with supportedEntryTypes even in 5.x versions.

Let me know your thoughts.

[devcorpio]

@vigneshshanmugam thanks for your comment.

It's true, PAINT started to be compatible on Safari 14.1. I wouldn't worried about Chrome either, even if PAINT was available on 60 and supportedEntryTypes on 73. I understand that we rely on Chrome being an evergreen browser and that allows us to be a bit more flexible with the browser support here.

🎉

[apmmachine]

🌐 Coverage report

Name	Metrics % (covered/total)	Diff
Packages	100.0% (8/8)	💚
Files	98.039% (50/51)	👎 -0.109
Classes	98.039% (50/51)	👎 -0.109
Methods	97.356% (405/416)	👎 -0.05
Lines	95.302% (2069/2171)	👍 0.001
Conditionals	86.421% (1031/1193)	👍 0.083

[devcorpio]

LGTM 🎉