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

[rum] added extra installation method in readme #571

Merged
merged 24 commits into from
Oct 23, 2020

Conversation

hdelaby
Copy link
Contributor

@hdelaby hdelaby commented Oct 16, 2020

Motivation

Release of async loading of the SDK

Changes

  • Added "Choosing the right installation method"
  • Added async code snippet

Testing

Only documentation modification.


I have gone over the contributing documentation.

@hdelaby hdelaby force-pushed the hdelaby/document-async-bundle branch from cff29cb to c06b158 Compare October 16, 2020 13:22
@hdelaby
Copy link
Contributor Author

hdelaby commented Oct 16, 2020

@bcaudan Few details I am not sure about:

  1. I want to remove the dd-action-name part from here, wdyt?
  2. What about renaming bundle to RUM snippet or code snippet? I feel like bundle isn't really mainstream (I left both on purpose in the code)

@bcaudan
Copy link
Contributor

bcaudan commented Oct 16, 2020

@hdelaby

  1. hum yes public API section has been trimmed in DOCS-1217 RUM Browser Monitoring #531, we could maybe add a specific .md for that

  2. FMU, bundle refers to the sdk file and snippet to the inline js code. I'd rather go with something like:

# setup
## NPM
## CDN async
## CDN sync

@codecov-io
Copy link

codecov-io commented Oct 20, 2020

Codecov Report

Merging #571 into master will increase coverage by 0.50%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #571      +/-   ##
==========================================
+ Coverage   88.13%   88.63%   +0.50%     
==========================================
  Files          46       46              
  Lines        2359     2359              
  Branches      492      492              
==========================================
+ Hits         2079     2091      +12     
+ Misses        280      268      -12     
Impacted Files Coverage Δ
packages/rum/src/browser/performanceCollection.ts 68.57% <0.00%> (+1.42%) ⬆️
packages/rum/src/transport/batch.ts 82.92% <0.00%> (+9.75%) ⬆️
packages/rum/src/domain/parentContexts.ts 95.58% <0.00%> (+10.29%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 818210e...2b013b3. Read the comment docs.

@hdelaby hdelaby marked this pull request as ready for review October 20, 2020 09:48
@hdelaby hdelaby requested review from a team as code owners October 20, 2020 09:48
### CDN sync

```html
<script src="https://www.datadoghq-browser-agent.com/datadog-logs.js">
Copy link
Member

Choose a reason for hiding this comment

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

This should be:

Suggested change
<script src="https://www.datadoghq-browser-agent.com/datadog-logs.js">
<script src="https://www.datadoghq-browser-agent.com/datadog-logs.js"></script>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add a prettier-ignore comment line otherwise I think this commit will break the prettier CI check for later commits.

packages/rum/README.md Outdated Show resolved Hide resolved
packages/logs/README.md Outdated Show resolved Hide resolved
Hugo Delaby and others added 5 commits October 20, 2020 14:40
Co-authored-by: Benoît Zugmeyer <benoit.zugmeyer@datadoghq.com>
Co-authored-by: Benoît Zugmeyer <benoit.zugmeyer@datadoghq.com>
Copy link
Contributor

@kayayarai kayayarai left a comment

Choose a reason for hiding this comment

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

docs look good

@@ -15,7 +15,7 @@ With the `datadog-logs` library, you can send logs directly to Datadog from JS c

**Datadog client token**: For security reasons, [API keys][1] cannot be used to configure the `datadog-logs` library, because they would be exposed client-side in the JavaScript code. To collect logs from web browsers, a [client token][2] must be used. See the [client token documentation][2] for more details.

**Datadog browser log library**: Configure the library through [NPM](#npm) or use the [bundle](#bundle) directly in the head tag.
**Datadog browser log library**: Configure the library through [NPM](#npm) or use the [CDN async](#cdn-async) or [CDN sync](#cdn-sync) code snippets in the head tag.
Copy link
Contributor

Choose a reason for hiding this comment

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

what about getting rid of all bundle references then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the normal doc you mean?

Copy link
Contributor

Choose a reason for hiding this comment

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

in this page at least, in other doc would be great as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My latest commit should have removed the last reference of bundle :)

@@ -133,22 +171,6 @@ init(configuration: {
})
```

### Name click actions

The RUM library uses various strategies to automatically name click actions. If you want more control, define a `data-dd-action-name` attribute on clickable elements (or any of their parents) to name the action, for example:
Copy link
Contributor

Choose a reason for hiding this comment

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

is it documented elsewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll open a PR to add it in the normal doc.

@ruthnaebeck ruthnaebeck removed their request for review October 22, 2020 20:29
Copy link
Contributor

@bcaudan bcaudan left a comment

Choose a reason for hiding this comment

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

LGTM!

@hdelaby hdelaby merged commit b0c414c into master Oct 23, 2020
@hdelaby hdelaby deleted the hdelaby/document-async-bundle branch October 23, 2020 08:59
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