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(instrumenation-document-load): Add custom attributes to document load #1414

Merged

Conversation

Abinet18
Copy link
Contributor

@Abinet18 Abinet18 commented Feb 28, 2023

Which problem is this PR solving?

Adding custom attributes to the Document load instrumentation spans (documentLoad, documentFetch, resourceFetch)
Cloned from #1238

Short description of the changes

This adds a configuration to the document load instrumentation which takes a map of span type to a function that can access the corresponding span and add some attributes to it. By providing the function as a config while registering the DocumentLoad instrumentation will allow for such addition of some vendor specific attributes to the spans.

@Abinet18 Abinet18 requested a review from a team February 28, 2023 21:42
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Feb 28, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: Abinet18 (31fa293)
  • ✅ login: mhassan1 / name: Marc Hassan (1a46b04)

@github-actions github-actions bot requested a review from obecny February 28, 2023 21:42
@Abinet18 Abinet18 changed the title Add custom attributes document load feat(instrumenation-document-load): Add custom attributes document load Feb 28, 2023
@Abinet18 Abinet18 changed the title feat(instrumenation-document-load): Add custom attributes document load feat(instrumenation-document-load): Add custom attributes to document load Feb 28, 2023
@Abinet18 Abinet18 force-pushed the AddCustomAttributes_DocumentLoad branch from 762ddb1 to 1a46b04 Compare February 28, 2023 22:08
@github-actions github-actions bot requested a review from rauno56 February 28, 2023 22:08
@Abinet18 Abinet18 force-pushed the AddCustomAttributes_DocumentLoad branch from 1a46b04 to 31fa293 Compare February 28, 2023 22:12
@Abinet18
Copy link
Contributor Author

@dyladan , @MSNev Can you review this PR instead of the previous one which I abandoned to have a clean start.

@martinkuba
Copy link
Contributor

Could this not be accomplished by using a custom processor?

@Abinet18
Copy link
Contributor Author

Abinet18 commented Mar 7, 2023

I see that this can be achieved with custom processors. The difference is that with custom processor , all spans will have to be intercepted and modified accordingly , filtering based on some span properties. This will only apply to document load instrumentation spans. There is a similar implementation for fetch and xhr to add custom attributes. If using custom processors is the choice for all scenarios, we may have to deprecate the fetch and xhr apply custom attributes functionality too.

@martinkuba
Copy link
Contributor

I see, makes sense since there is already a precedence for this.

All the other instrumentations use the name applyCustomAttributesOnSpan. I would recommend that we use the same name for consistency.

@Abinet18
Copy link
Contributor Author

Abinet18 commented Mar 9, 2023

Can this be merged ?

@codecov
Copy link

codecov bot commented Mar 9, 2023

Codecov Report

Merging #1414 (9ab5257) into main (278ba99) will increase coverage by 0.06%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1414      +/-   ##
==========================================
+ Coverage   96.06%   96.13%   +0.06%     
==========================================
  Files          14       14              
  Lines         890      906      +16     
  Branches      192      197       +5     
==========================================
+ Hits          855      871      +16     
  Misses         35       35              
Impacted Files Coverage Δ
...etapackages/auto-instrumentations-web/src/utils.ts 95.83% <100.00%> (ø)
...strumentation-document-load/src/instrumentation.ts 98.98% <100.00%> (+0.19%) ⬆️

@martinkuba martinkuba merged commit 98609c6 into open-telemetry:main Mar 22, 2023
@dyladan dyladan mentioned this pull request Mar 22, 2023
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