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: added tags to lana logs #1463

Merged
merged 14 commits into from
Oct 31, 2023
Merged

Conversation

robert-bogos
Copy link
Contributor

@robert-bogos robert-bogos commented Oct 27, 2023

Description

This PR addresses the requirement of enabling filtering capabilities for Splunk logs based on tags from LANA logs.

Recommended tags:
info - network issues
warn - authoring related misconfigs or similar
error - actual error ( ex. cannot read Y of undefind )
<module>- extra tag to help centralise all logs related to a specific module

Related Issue

Resolves: MWPW-138031

Testing instructions

  1. Change the index value in the Splunk search to lana_nonprod
  2. Add a tags key with an array of strings as value to the json sent as argument for the lanaLog method
    or if you're using the logErrorFor method add the array of strings as the third argument ( the strings in the array are the tags that will be associated with the log )
    E.g.
    lanaLog({ message: 'Could not create global navigation.', e, tags: 'errorType=error,module=gnav' });
    logErrorFor(onToggleClick, 'Toggle click failed', 'errorType=error,module=gnav' ));
  3. All the logs sent from your local environment should show in Splunk

Screenshots:

Screenshot 2023-10-27 at 16 31 50

Test URLs

Acrobat:

BACOM:

CC:

Milo:

@robert-bogos robert-bogos added run-nala Run Nala Test Automation against PR needs-verification PR requires E2E testing by a reviewer labels Oct 27, 2023
@aem-code-sync
Copy link
Contributor

aem-code-sync bot commented Oct 27, 2023

Hello, I'm the AEM Code Sync Bot and I will run some test suites that validate the page speed.
In case there are problems, just click the checkbox below to rerun the respective action.

  • Re-run PSI Checks

@codecov
Copy link

codecov bot commented Oct 27, 2023

Codecov Report

Merging #1463 (b81ae4a) into main (f3b836c) will decrease coverage by 0.02%.
The diff coverage is 84.00%.

@@            Coverage Diff             @@
##             main    #1463      +/-   ##
==========================================
- Coverage   95.61%   95.60%   -0.02%     
==========================================
  Files         146      146              
  Lines       37691    37692       +1     
==========================================
- Hits        36038    36034       -4     
- Misses       1653     1658       +5     
Files Coverage Δ
...bal-navigation/features/breadcrumbs/breadcrumbs.js 100.00% <100.00%> (ø)
...cks/global-navigation/features/profile/dropdown.js 97.28% <100.00%> (ø)
...s/global-navigation/features/search/gnav-search.js 95.48% <100.00%> (ø)
...ocks/global-navigation/utilities/keyboard/index.js 87.96% <100.00%> (ø)
...ks/global-navigation/utilities/keyboard/mainNav.js 91.25% <100.00%> (ø)
...lobal-navigation/utilities/keyboard/mobilePopup.js 97.91% <100.00%> (ø)
...ocks/global-navigation/utilities/keyboard/popup.js 97.54% <100.00%> (ø)
...bs/blocks/global-navigation/utilities/menu/menu.js 98.31% <100.00%> (ø)
...bs/blocks/global-navigation/utilities/utilities.js 100.00% <100.00%> (+0.97%) ⬆️
libs/blocks/global-navigation/global-navigation.js 96.82% <55.55%> (ø)

... and 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@mokimo mokimo left a comment

Choose a reason for hiding this comment

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

The linter has a few complaints

@robert-bogos
Copy link
Contributor Author

The linter has a few complaints

Fixed, I was using another formatter

Copy link
Contributor

@mokimo mokimo left a comment

Choose a reason for hiding this comment

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

Awesome first PR 🚀 If you have issues with the LH score checks, you can also use .live links with published pages - those checks are usually a bit more stable than the .page links since they sit behind a CDN.

One suggestion to keep the code a little leaner. Additionally you could add the tag description to your PR description

// Common used LANA logs tags:
// info - network issues
// warn - authoring related misconfigs or similar
// error - actual error ( ex. cannot read Y of undefind )
// <module> - extra tag to help centralise all logs related to a specific module

libs/blocks/global-navigation/utilities/utilities.js Outdated Show resolved Hide resolved
libs/blocks/global-navigation/utilities/utilities.js Outdated Show resolved Hide resolved
@mokimo mokimo requested a review from amauch-adobe October 27, 2023 14:57
@adobecom adobecom deleted a comment from aem-code-sync bot Oct 27, 2023
@adobecom adobecom deleted a comment from aem-code-sync bot Oct 27, 2023
@adobecom adobecom deleted a comment from aem-code-sync bot Oct 27, 2023
@adobecom adobecom deleted a comment from aem-code-sync bot Oct 27, 2023
@narcis-radu narcis-radu self-assigned this Oct 27, 2023
Copy link
Contributor

@amauch-adobe amauch-adobe left a comment

Choose a reason for hiding this comment

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

Approved from my standpoint, this will help us granulate alerts as well since we can now query on tags.

@skumar09 skumar09 added run-nala Run Nala Test Automation against PR and removed run-nala Run Nala Test Automation against PR labels Oct 29, 2023
@adobecom adobecom deleted a comment from aem-code-sync bot Oct 30, 2023
libs/blocks/global-navigation/features/profile/dropdown.js Outdated Show resolved Hide resolved
libs/blocks/global-navigation/global-navigation.js Outdated Show resolved Hide resolved
libs/blocks/global-navigation/global-navigation.js Outdated Show resolved Hide resolved
libs/blocks/global-navigation/global-navigation.js Outdated Show resolved Hide resolved
libs/blocks/global-navigation/global-navigation.js Outdated Show resolved Hide resolved
libs/blocks/global-navigation/global-navigation.js Outdated Show resolved Hide resolved
libs/blocks/global-navigation/global-navigation.js Outdated Show resolved Hide resolved
libs/blocks/global-navigation/global-navigation.js Outdated Show resolved Hide resolved
libs/blocks/global-navigation/global-navigation.js Outdated Show resolved Hide resolved
libs/blocks/global-navigation/global-navigation.js Outdated Show resolved Hide resolved
@narcis-radu narcis-radu self-requested a review October 30, 2023 12:23
@adobecom adobecom deleted a comment from aem-code-sync bot Oct 30, 2023
@adobecom adobecom deleted a comment from aem-code-sync bot Oct 30, 2023
@narcis-radu narcis-radu self-requested a review October 30, 2023 12:35
@robert-bogos robert-bogos requested a review from mokimo October 30, 2023 16:41
@adobecom adobecom deleted a comment from aem-code-sync bot Oct 30, 2023
@adobecom adobecom deleted a comment from aem-code-sync bot Oct 30, 2023
@adobecom adobecom deleted a comment from aem-code-sync bot Oct 30, 2023
Copy link
Contributor

@mokimo mokimo left a comment

Choose a reason for hiding this comment

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

👍

Co-authored-by: Okan Sahin <39759830+mokimo@users.noreply.github.com>
@adobecom adobecom deleted a comment from aem-code-sync bot Oct 31, 2023
@adobecom adobecom deleted a comment from aem-code-sync bot Oct 31, 2023
Copy link
Contributor

@overmyheadandbody overmyheadandbody left a comment

Choose a reason for hiding this comment

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

Great job on your first PR! 👏

@aem-code-sync
Copy link
Contributor

aem-code-sync bot commented Oct 31, 2023

Page Scores Audits Google
/drafts/rbogos/default?martech=off PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI

@adobecom adobecom deleted a comment from aem-code-sync bot Oct 31, 2023
Comment on lines +175 to +178
await logErrorFor(erroneousFunction, 'message', 'someTags');

// Check if lanaLog (through window.lana.log) was called with expected parameters.
expect(lanaLogSpy.calledOnce).to.be.true;
Copy link
Contributor

Choose a reason for hiding this comment

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

You could expand the check to see that the tags and messages are also correctly called

@narcis-radu narcis-radu removed the needs-verification PR requires E2E testing by a reviewer label Oct 31, 2023
@narcis-radu narcis-radu merged commit cdc1863 into adobecom:main Oct 31, 2023
10 of 12 checks passed
vgoodric pushed a commit to vgoodric/bootcamp-milo that referenced this pull request Feb 1, 2024
* feat: added tags to lana logs

* hotfix: formatting

* hotfix: formatting

* hotfix: removed composeLanaTags method

* hotfix: gnav label for global-navigation

* hotfix: send string instead of array to splunk

* Update libs/blocks/global-navigation/features/search/gnav-search.js

Co-authored-by: Okan Sahin <39759830+mokimo@users.noreply.github.com>

* feat: unit test for lana logs

---------

Co-authored-by: Aaron Mauchley <mauchley@adobe.com>
Co-authored-by: Okan Sahin <39759830+mokimo@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
run-nala Run Nala Test Automation against PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants