-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Add namespace field to activation log #4609
Add namespace field to activation log #4609
Conversation
LGTM but please send to dev list. |
ok, I sent the email |
reopen to trigger the travis check |
@jiangpengcheng it seems that Travis is frequently failing on this PR - I guess the system test part? I restarted only the system test part for you because all other Travis tests have passed. I also had the situation for a lot of my PRs that 3 of 4 Travis checks passed successfully - but only system test was failing. I wonder whether we have an interference between tests such that the system test will always / very likely fail if the other Travis tests are running as well... |
Codecov Report
@@ Coverage Diff @@
## master #4609 +/- ##
==========================================
- Coverage 84.44% 78.76% -5.68%
==========================================
Files 183 183
Lines 8306 8350 +44
Branches 572 564 -8
==========================================
- Hits 7014 6577 -437
- Misses 1292 1773 +481
Continue to review full report at Codecov.
|
I'm also ok with the changes introduced by this PR. At the same time, I see that the different classes for providing action container (aka. user) logs are diverging. We have two different strategies to persist user logs in a log store:
The This pull request changes Line 58 in d4d7ceb
This pull request should probably also cover At the same time, we cannot expect that this pull request addresses the code duplication of diverging log processing approaches... |
@sven-lange-last and about the checks, the result is strange because all cases are passed in my local env, I didn;t know the reason. Since close and reopen PRs seems kind of ugly, I wonder whether it's ok to add some "comment" triggers to re-trigger travis build by contributors themself? e.g. "build system", "build unittest". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will a sequence activation have the same set of extra annotations?
@rabbah sorry, I don't get it, what's the |
Take a look at |
8b8b694
to
37ae815
Compare
I think this PR doesn't related to any annotations but just add a |
@jiangpengcheng please give me some time to check your latest updates... |
JsObject( | ||
Map("type" -> "user_log".toJson) ++ Map("message" -> log.toJson) ++ Map( | ||
"activationId" -> activation.activationId.toJson) ++ Map( | ||
"namespace" -> activation.namespace.asString.toJson) ++ additionalFields) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for attending to my feedback and also covering other log stores.
At the same time: can you please move the namespace
extension over to ArtifactWithFileStorageActivationStore
? This would be the equivalent class to DockerToActivationFileLogStore
where you inject namespace
via additionalMetadata
.
ActivationFileStorage
is just a helper that performs writing of logs and activations to a file. The log store implementations that make use of ActivationFileStorage
should decide which additional metadata they want to add. ArtifactWithFileStorageActivationStore
is such a log store.
See
Lines 58 to 60 in 400a791
val additionalFields = Map(config.userIdField -> context.user.namespace.uuid.toJson) | |
activationFileStorage.activationToFile(activation, context, additionalFields) |
^^ I suggest to inject the additional namespace
field here.
Hint: IBM is using the ActivationFileStorage
in a private implementation for IBM's logging system that requires different fields unique to IBM that do not make sense in the openwhisk context. That's why we want to keep ActivationFileStorage
as open as possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess you also need to update this line in the test to inject the namespace
field:
Line 280 in 37ae815
val additionalFields = Map(config.userIdField -> context.user.namespace.uuid.toJson) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but then activation will also has an extra "namespace" field, is that ok?
openwhisk/common/scala/src/main/scala/org/apache/openwhisk/core/database/ActivationFileStorage.scala
Line 116 in 400a791
activationToFileExtended(activation, context, additionalFields, additionalFields) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's have a look at the code:
Lines 55 to 62 in 400a791
override def store(activation: WhiskActivation, context: UserContext)( | |
implicit transid: TransactionId, | |
notifier: Option[CacheChangeNotification]): Future[DocInfo] = { | |
val additionalFields = Map(config.userIdField -> context.user.namespace.uuid.toJson) | |
activationFileStorage.activationToFile(activation, context, additionalFields) | |
super.store(activation, context) | |
} |
If you add the namespace
field to the additionalFields
map in line 58, it will be persisted to logs via activationFileStorage.activationToFile()
in line 60.
The additionalFields
map won't be considered when storing the activation to the database (line 61).
Do you come to the same conclusion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
what I mean is that when write activation to file, it will also has an additional field "namespace"(alghough this just override the original "namespace" filed of activation):
openwhisk/common/scala/src/main/scala/org/apache/openwhisk/core/database/ActivationFileStorage.scala
Lines 113 to 126 in 400a791
def activationToFile(activation: WhiskActivation, | |
context: UserContext, | |
additionalFields: Map[String, JsValue] = Map.empty): Unit = { | |
activationToFileExtended(activation, context, additionalFields, additionalFields) | |
} | |
// used by external ArtifactActivationStore SPI implementation | |
def activationToFileExtended(activation: WhiskActivation, | |
context: UserContext, | |
additionalFieldsForLogs: Map[String, JsValue] = Map.empty, | |
additionalFieldsForActivation: Map[String, JsValue] = Map.empty, | |
includeResult: Boolean = writeResultToFile): Unit = { | |
val transcribedLogs = transcribeLogs(activation, additionalFieldsForLogs) | |
val transcribedActivation = transcribeActivation(activation, additionalFieldsForActivation, includeResult) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We clarified the open questions in a personal Slack communication and agreed on a solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for applying recent changes to ArtifactWithFileStorageActivationStore
. Changes look good to me now.
Unfortunately, there still is a code formatting issue...
> Task :common:scala:checkScalafmt FAILED
FAILURE: Build failed with an exception.
* What went wrong:
Execution failed for task ':common:scala:checkScalafmt'.
> Files incorrectly formatted: /home/travis/build/apache/openwhisk/common/scala/src/main/scala/org/apache/openwhisk/core/database/ArtifactWithFileStorageActivationStore.scala
* Try:
Run with --stacktrace option to get the stack trace. Run with --info or --debug option to get more log output. Run with --scan to get full insights.
* Get more help at https://help.gradle.org
BUILD FAILED in 1m 17s
oh, sorry, I missed this file in last commit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for attend to all feedback. Ready to be merged from my point of view. @rabbah are you ok with merging?
@jiangpengcheng do you want this change to be merged or do you want to add anything?
nothing to update here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had misunderstood this PR initially as adding a new annotation to the WhiskActivation document (to record the namespace). Thanks Sven for clarifying the change for me. I defer to you on readiness. The changes LGTM.
Add the activation namespace to all activation log lines forwarded to external logging solutions.
Add namespace field to activation log while using
DockerToActivationFileLogStore
Description
Currently there is no namespace field but a namespaceId in the activation log while save activation logs to a separate file, while I think add namesapce will be more convenient for users when they process the log file, such as using Logstash to send logs to different indices(e.g.
openwhisk-%{namespace}
) in ElasticSearchRelated issue and scope
My changes affect the following components
Types of changes
Checklist: