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(rum-core): add apmRequest config typescript type #1254

Merged
merged 2 commits into from Jul 4, 2022

Conversation

ReneWerner87
Copy link
Contributor

@hmdhk you had forgotten the typescript files, can you please check if it fits like this

the feature is great and we use it, thanks for your effort

@cla-checker-service
Copy link

cla-checker-service bot commented Jul 1, 2022

💚 CLA has been signed

@apmmachine
Copy link
Collaborator

apmmachine commented Jul 1, 2022

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2022-07-04T14:56:13.001+0000

  • Duration: 77 min 1 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!)

@ReneWerner87
Copy link
Contributor Author

/test

@devcorpio
Copy link
Contributor

Hi @ReneWerner87,

Thanks for your contribution!

TL;DR:

The types have to be a bit different:

    apmRequest?: (
      requestParams: {
        xhr: XMLHttpRequest
        url: string
        method: string
        payload: string | undefined
        headers: Record<string, string> | undefined
      },
    ) => boolean

Context:

When apmRequest is set the agent execute it on two different situations:

  1. when fetching central config. This only happens when centralConfig is set to true.
  2. when sending events/beacons to the APM server

On the case of the central config the arguments the agent provides are xhr, url, and method. (payload and headers are undefined).

On the case of the events/beacons the payload and headers arguments are provided, but payload type is a string and headers is an object literal.

@devcorpio devcorpio changed the title typescript support for "feat(rum-core): add apmServerRequest config #1019" feat(rum-core): add apmRequest config typescript type Jul 4, 2022
@ReneWerner87
Copy link
Contributor Author

Hi @ReneWerner87,

Thanks for your contribution!

TL;DR:

The types have to be a bit different:

    apmRequest?: (
      requestParams: {
        xhr: XMLHttpRequest
        url: string
        method: string
        payload: string | undefined
        headers: Record<string, string> | undefined
      },
    ) => boolean

Context:

When apmRequest is set the agent execute it on two different situations:

  1. when fetching central config. This only happens when centralConfig is set to true.
  2. when sending events/beacons to the APM server

On the case of the central config the arguments the agent provides are xhr, url, and method. (payload and headers are undefined).

On the case of the events/beacons the payload and headers arguments are provided, but payload type is a string and headers is an object literal.

thx, i changed the types

@apmmachine
Copy link
Collaborator

📦 Bundlesize report

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

@apmmachine
Copy link
Collaborator

🌐 Coverage report

Name Metrics % (covered/total) Diff
Packages 100.0% (8/8) 💚
Files 98.148% (53/54) 👍 0.109
Classes 98.148% (53/54) 👍 0.109
Methods 97.406% (413/424) 👍 0.05
Lines 95.26% (2110/2215) 👎 -0.042
Conditionals 86.278% (1050/1217) 👎 -0.143

Copy link
Contributor

@devcorpio devcorpio left a comment

Choose a reason for hiding this comment

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

LGTM! 🎉 , This will be part of the next release we do.

@devcorpio devcorpio merged commit cfaa327 into elastic:main Jul 4, 2022
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.

None yet

3 participants