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

fix(MWPW-155964):Lana log error type changed to info. #2818

Merged
merged 9 commits into from
Sep 12, 2024

Conversation

sharath-kannan
Copy link
Contributor

@sharath-kannan sharath-kannan requested a review from a team as a code owner September 3, 2024 06:31
Copy link
Contributor

aem-code-sync bot commented Sep 3, 2024

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

Copy link

codecov bot commented Sep 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.19%. Comparing base (510f7a6) to head (7bc712e).
Report is 20 commits behind head on stage.

Additional details and impacted files
@@            Coverage Diff             @@
##            stage    #2818      +/-   ##
==========================================
+ Coverage   95.87%   96.19%   +0.32%     
==========================================
  Files         173      215      +42     
  Lines       46333    53992    +7659     
==========================================
+ Hits        44421    51940    +7519     
- Misses       1912     2052     +140     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -118,14 +118,14 @@ export const getTargetPersonalization = async () => {
const responseStart = Date.now();
window.addEventListener(ALLOY_SEND_EVENT, () => {
const responseTime = calculateResponseTime(responseStart);
window.lana.log(`target response time: ${responseTime}`, { tags: 'errorType=info,module=martech' });
window.lana.log(`target response time: ${responseTime}`, { tags: 'module-martech', errorType: 'i' });
Copy link
Contributor

Choose a reason for hiding this comment

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

is module-martech tag existing? wouldn't martech be clear enough, yet smaller?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I will make this change.

}, { once: true });

let manifests = [];
let propositions = [];
const response = await waitForEventOrTimeout(ALLOY_SEND_EVENT, timeout);
if (response.error) {
window.lana.log('target response time: ad blocker', { tags: 'errorType=info,module=martech' });
window.lana.log('target response time: ad blocker', { tags: 'errorType-info,module-martech' });
Copy link
Contributor

Choose a reason for hiding this comment

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

isn't this not consistent with the above? shouldn't it be
{ tags: 'module-martech', errorType: 'i' } too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should be, if the log was informational but since this is clearly inside the error block i have left the errorType to default which is "error".

Copy link
Contributor

Choose a reason for hiding this comment

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

mm to me "error" is a problem on our side, the fact the browser uses adblocker does not really fall into that category, but i guess i'm fine :)

libs/martech/martech.js Outdated Show resolved Hide resolved
@JackySun9 JackySun9 added the run-nala Run Nala Test Automation against PR label Sep 3, 2024
Copy link
Contributor

github-actions bot commented Sep 4, 2024

This pull request is not passing all required checks. Please see this discussion for information on how to get all checks passing. Inconsistent checks can be manually retried. If a test absolutely can not pass for a good reason, please add a comment with an explanation to the PR.

Copy link
Contributor

@thedoc31 thedoc31 left a comment

Choose a reason for hiding this comment

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

key-value pairs are not supported in the tags. Please refer to https://wiki.corp.adobe.com/display/WCMSOps/LANA+-+Log+Always+Never+Assume#LANALogAlwaysNeverAssume-Parameters for the correct parameters to pass to LANA.

I was wrong, see comment below.

Copy link
Contributor

@thedoc31 thedoc31 left a comment

Choose a reason for hiding this comment

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

This format should work, per the documentation. Please validate in Splunk that these messages now come in as info type.

Copy link
Contributor

@robert-bogos robert-bogos left a comment

Choose a reason for hiding this comment

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

Hi @sharath-kannan ! I think the Jira ticket aimed to correct all the LANA logs that are not being used properly, not just the ones from martech.

@sharath-kannan
Copy link
Contributor Author

Hi @sharath-kannan ! I think the Jira ticket aimed to correct all the LANA logs that are not being used properly, not just the ones from martech.

Okay,I will correct all the lana logs that are used incorrectly in milo and update my PR. Hope using "module-" will be fine for now?

@thedoc31
Copy link
Contributor

thedoc31 commented Sep 4, 2024

Hi @sharath-kannan ! I think the Jira ticket aimed to correct all the LANA logs that are not being used properly, not just the ones from martech.

Okay,I will correct all the lana logs that are used incorrectly in milo and update my PR. Hope using "module-" will be fine for now?

I would recommend making the tags as concise as possible, but a dash character is fine within a comma-delimited list. The restricted characters are documented on the wiki [1] in the note above the table.

[1]
https://wiki.corp.adobe.com/display/WCMSOps/LANA+-+Log+Always+Never+Assume#LANALogAlwaysNeverAssume-Parameters

@spadmasa spadmasa self-assigned this Sep 5, 2024
@skumar09 skumar09 added run-nala Run Nala Test Automation against PR and removed run-nala Run Nala Test Automation against PR labels Sep 6, 2024
@skumar09
Copy link
Contributor

skumar09 commented Sep 6, 2024

@sharath-kannan : can you please update your branch with latest code base( I guess it doesn't have video block changes, as corresponding nala tests are failing)

Copy link
Contributor

@hparra hparra left a comment

Choose a reason for hiding this comment

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

It's not clear why we're doing this. Did the LANA interface change? At the time this was correct.

video-metadata is a deprecated component so there is no issue here.

libs/blocks/faas/utils.js Outdated Show resolved Hide resolved
@thedoc31
Copy link
Contributor

thedoc31 commented Sep 9, 2024

It's not clear why we're doing this. Did the LANA interface change? At the time this was correct.

video-metadata is a deprecated component so there is no issue here.

Unfortunately, the information provided at the time was incorrect. Key-value pairs have never been supported in tags: elements. The documentation has been updated to make this very clear.

@spadmasa
Copy link

@spadmasa spadmasa added verified PR has been E2E tested by a reviewer Ready for Stage labels Sep 11, 2024
@milo-pr-merge milo-pr-merge bot merged commit a7c31b7 into adobecom:stage Sep 12, 2024
21 checks passed
@milo-pr-merge milo-pr-merge bot mentioned this pull request Sep 12, 2024
yesil pushed a commit that referenced this pull request Sep 18, 2024
* lana log error type changed

* modified lana logs

* added a few unit tests for lana logs

* Updated tag in  utils.js

* linting error fixed

* linting error fix 2

* unit test fix

* added unit test for vimeo
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready for Stage run-nala Run Nala Test Automation against PR verified PR has been E2E tested by a reviewer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants