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/ub 1325 adda action name to the logs #219

Merged
merged 19 commits into from
Jul 15, 2018

Conversation

olgashtivelman
Copy link
Collaborator

@olgashtivelman olgashtivelman commented Jun 28, 2018

this PR will add an action name to each log line. (for example all oprerations that are part of Delete will have the request-action name in the log.
like : fd5dd389-7acb-11e8-88cd-54ee75515403-Delete


This change is Reviewable

@coveralls
Copy link

coveralls commented Jun 28, 2018

Coverage Status

Coverage decreased (-0.08%) to 51.398% when pulling 602ff4e on fix/UB-1325_adda_action_name_to_the_logs into 250d071 on dev.

@shay-berman
Copy link
Contributor

Review status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @olgashtivelman and @shay-berman)


utils/logs/go_logging_logger.go, line 86 at r3 (raw file):

	if l.params.ShowGoid{
		return fmt.Sprintf("%s:%d-%s", new_context.Id, go_id, new_context.ActionName)

whats happen if action name not given in the context?
I think we should succeed to work but without show the action name.


Comments from Reviewable

Copy link
Collaborator Author

@olgashtivelman olgashtivelman left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @olgashtivelman and @shay-berman)


utils/logs/go_logging_logger.go, line 86 at r3 (raw file):

Previously, shay-berman wrote…

whats happen if action name not given in the context?
I think we should succeed to work but without show the action name.

a struct will always have a default value to parameters it will just be empty. do you mean that you have a problem with it looking like : "id-" ?

Copy link
Contributor

@shay-berman shay-berman left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @shay-berman)


utils/logs/go_logging_logger.go, line 86 at r3 (raw file):

Previously, olgashtivelman wrote…

a struct will always have a default value to parameters it will just be empty. do you mean that you have a problem with it looking like : "id-" ?

yes it may be strange
but also what if old ubiquity flex will work with the new ubiqutiy server.
we need to think if we have some issue in that use case.

Copy link
Collaborator Author

@olgashtivelman olgashtivelman left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @olgashtivelman)


utils/logs/go_logging_logger.go, line 86 at r3 (raw file):

Previously, shay-berman wrote…

yes it may be strange
but also what if old ubiquity flex will work with the new ubiqutiy server.
we need to think if we have some issue in that use case.

@shay-berman OK but I don't understand what it is that you want me to change. do you want to check if the action name is not empty and if it is to set it to NA?

do we support old ubiquity-flex with new ubiquity server?
is anyone checking this?
there were other changes with the logging which assume that flex+ubiquity are in sync. also the requests are different so how can this be supported at all?

Copy link
Collaborator Author

@olgashtivelman olgashtivelman left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @shay-berman)


utils/logs/go_logging_logger.go, line 86 at r3 (raw file):

Previously, olgashtivelman wrote…

@shay-berman OK but I don't understand what it is that you want me to change. do you want to check if the action name is not empty and if it is to set it to NA?

do we support old ubiquity-flex with new ubiquity server?
is anyone checking this?
there were other changes with the logging which assume that flex+ubiquity are in sync. also the requests are different so how can this be supported at all?

Done.

Copy link
Contributor

@shay-berman shay-berman left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 2 of 6 files at r3, 1 of 1 files at r4.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@olgashtivelman olgashtivelman merged commit 490b484 into dev Jul 15, 2018
@olgashtivelman olgashtivelman deleted the fix/UB-1325_adda_action_name_to_the_logs branch July 15, 2018 07:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants