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

EBP-24: As a developer, I can retrieve the cache request ID from a cached message #78

Merged
merged 3 commits into from
Jan 7, 2025

Conversation

oodigie
Copy link

@oodigie oodigie commented Dec 19, 2024

Changes:

  • EBP-24: As a developer, I can retrieve the cache request ID from a cached message

Copy link

gitstream-cm bot commented Dec 19, 2024

Please mark whether you used Copilot to assist coding in this PR

  • Copilot Assisted

@oodigie oodigie requested a review from sdewilde December 19, 2024 14:25
Copy link

@sdewilde sdewilde left a comment

Choose a reason for hiding this comment

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

Tests look good to me with the functionality we have so far!

cacheID, errInfo := ccsmp.SolClientMessageGetCacheRequestID(inboundMessage.messagePointer)
if errInfo != nil {
if errInfo.ReturnCode == ccsmp.SolClientReturnCodeFail {
logging.Default.Debug(fmt.Sprintf("Encountered error retrieving Cache ID: %s, subcode: %d", errInfo.GetMessageAsString(), errInfo.SubCode))

Choose a reason for hiding this comment

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

@oodigie , based on the API and Application Log Levels confluence page, this log seems to be informing the application that

An unexpected event or occurrence that does not affect the sane operation of the SDK or application.

so I would expect this to be a notice log. This would be coerced into an info log in the case of the Go API since AFAIK we do not have notice logs. I think that the application would want to be notified at a higher level of logging than debug if their operation failed. Please either make this change or let me know if you have a different opinion.

Copy link
Author

Choose a reason for hiding this comment

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

@TrentDaniel, I took a look at that confluence page and I think you are right. I will update this now.

Copy link

@TrentDaniel TrentDaniel left a comment

Choose a reason for hiding this comment

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

Requested some changes, also in the middle of a conversation with Ragnar about CacheRequestID, still not sure if its conclusion will be a change request.

@oodigie oodigie dismissed TrentDaniel’s stale review January 6, 2025 18:30

I have made the suggested change for the log level from Debug to Info and responded to the other comment.

Copy link

@TrentDaniel TrentDaniel left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link

@RagnarPaulson RagnarPaulson left a comment

Choose a reason for hiding this comment

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

All good.

@oodigie oodigie merged commit b308499 into SOL-62456 Jan 7, 2025
5 checks passed
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.

4 participants