Add namespace field to activation log by jiangpengcheng · Pull Request #4609 · apache/openwhisk

jiangpengcheng · 2019-09-06T01:36:01Z

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 ElasticSearch

Related issue and scope

I opened an issue to propose and discuss this change (#????)

My changes affect the following components

Types of changes

Bug fix (generally a non-breaking change which closes an issue).
Enhancement or new feature (adds new functionality).
Breaking change (a bug fix or enhancement which changes existing behavior).

Checklist:

I signed an Apache CLA.
I reviewed the style guides and followed the recommendations (Travis CI will check :).
I added tests to cover my changes.
My changes require further changes to the documentation.
I updated the documentation where necessary.

rabbah · 2019-09-06T01:41:05Z

LGTM but please send to dev list.

jiangpengcheng · 2019-09-06T01:50:44Z

ok, I sent the email

jiangpengcheng · 2019-09-06T07:41:23Z

reopen to trigger the travis check

sven-lange-last · 2019-09-09T11:00:12Z

@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-io · 2019-09-09T11:42:57Z

Codecov Report

Merging #4609 into master will decrease coverage by 5.67%.
The diff coverage is 100%.

@@            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

Impacted Files	Coverage Δ
...abase/ArtifactWithFileStorageActivationStore.scala	`91.66% <100%> (+0.75%)`	⬆️
...rpool/logging/DockerToActivationFileLogStore.scala	`68.08% <100%> (ø)`	⬆️
...core/database/cosmosdb/RxObservableImplicits.scala	`0% <0%> (-100%)`	⬇️
...ore/database/cosmosdb/cache/CacheInvalidator.scala	`0% <0%> (-100%)`	⬇️
...core/database/cosmosdb/CosmosDBArtifactStore.scala	`0% <0%> (-95.89%)`	⬇️
...tabase/cosmosdb/cache/CacheInvalidatorConfig.scala	`0% <0%> (-94.74%)`	⬇️
...sk/core/database/cosmosdb/CosmosDBViewMapper.scala	`0% <0%> (-92.6%)`	⬇️
...e/database/cosmosdb/cache/ChangeFeedListener.scala	`0% <0%> (-86.67%)`	⬇️
...e/database/cosmosdb/cache/KafkaEventProducer.scala	`0% <0%> (-76.48%)`	⬇️
...whisk/core/database/cosmosdb/CosmosDBSupport.scala	`0% <0%> (-74.08%)`	⬇️
... and 23 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 400a791...5ee2fc6. Read the comment docs.

sven-lange-last · 2019-09-09T11:44:25Z

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 LogStore implementation extracts user logs from the action container (https://github.com/apache/openwhisk/blob/master/common/scala/src/main/scala/org/apache/openwhisk/core/containerpool/logging/LogStore.scala) and returns the extracted logs (collectLogs). As a side effect of the extraction, logs are forwarded to the supported log store. This is what DockerToActivationFileLogStore is doing.
The LogStore implementation extracts user logs from the action container (https://github.com/apache/openwhisk/blob/master/common/scala/src/main/scala/org/apache/openwhisk/core/containerpool/logging/LogStore.scala) and returns the extracted logs (collectLogs). There is no forwarding to a log store (example: DockerToActivationLogStore). The extracted logs are attached to the activation.

The ArtifactActivationStore is responsible for persisting activations (https://github.com/apache/openwhisk/blob/master/common/scala/src/main/scala/org/apache/openwhisk/core/database/ArtifactActivationStore.scala). As a side effect of storing the activation, logs are transformed and forwarded to a log store. With this approach, parts of the activation itself are also forwarded to a log store. Example: ArtifactWithFileStorageActivationStore.

This pull request changes DockerToActivationFileLogStore while it does not cover ArtifactWithFileStorageActivationStore - see this code location:

openwhisk/common/scala/src/main/scala/org/apache/openwhisk/core/database/ArtifactWithFileStorageActivationStore.scala

Line 58 in d4d7ceb

    
           val additionalFields = Map(config.userIdField -> context.user.namespace.uuid.toJson)

This pull request should probably also cover ArtifactWithFileStorageActivationStore.

At the same time, we cannot expect that this pull request addresses the code duplication of diverging log processing approaches...

jiangpengcheng · 2019-09-10T03:43:37Z

@sven-lange-last
thanks for the review, I wasn't aware of the ArtifactWithFileStorageActivationStore before, and seems it does almost the same thing as the DockerToActivationFileLogStore

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".

rabbah

Will a sequence activation have the same set of extra annotations?

jiangpengcheng · 2019-09-16T02:45:17Z

Will a sequence activation have the same set of extra annotations?

@rabbah sorry, I don't get it, what's the annotations do you mean?

rabbah · 2019-09-20T01:00:24Z

Take a look at makeSequenceActivation in SequenceActions.

jiangpengcheng · 2019-09-20T07:12:24Z

I think this PR doesn't related to any annotations but just add a namespace field while write activations/activation logs to a separate file, a "sequence activation" will not do that AFAIK

sven-lange-last · 2019-09-20T08:32:08Z

@jiangpengcheng please give me some time to check your latest updates...

sven-lange-last · 2019-09-20T09:00:52Z

+        JsObject(
+          Map("type" -> "user_log".toJson) ++ Map("message" -> log.toJson) ++ Map(
+            "activationId" -> activation.activationId.toJson) ++ Map(
+            "namespace" -> activation.namespace.asString.toJson) ++ additionalFields)


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

openwhisk/common/scala/src/main/scala/org/apache/openwhisk/core/database/ArtifactWithFileStorageActivationStore.scala

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.

I guess you also need to update this line in the test to inject the namespace field:

openwhisk/tests/src/test/scala/org/apache/openwhisk/core/database/ArtifactWithFileStorageActivationStoreTests.scala

Line 280 in 37ae815

val additionalFields = Map(config.userIdField -> context.user.namespace.uuid.toJson)

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)

Let's have a look at the code:

openwhisk/common/scala/src/main/scala/org/apache/openwhisk/core/database/ArtifactWithFileStorageActivationStore.scala

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?

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)

We clarified the open questions in a personal Slack communication and agreed on a solution.

sven-lange-last

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

jiangpengcheng · 2019-09-24T01:32:47Z

oh, sorry, I missed this file in last commit

sven-lange-last

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?

jiangpengcheng · 2019-09-25T09:53:03Z

nothing to update here

rabbah

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.

rabbah approved these changes Sep 6, 2019

View reviewed changes

jiangpengcheng closed this Sep 6, 2019

jiangpengcheng reopened this Sep 6, 2019

jiangpengcheng closed this Sep 9, 2019

jiangpengcheng reopened this Sep 9, 2019

jiangpengcheng closed this Sep 9, 2019

jiangpengcheng reopened this Sep 9, 2019

rabbah reviewed Sep 12, 2019

View reviewed changes

rabbah mentioned this pull request Sep 12, 2019

OpenWhisk User Events #4584

Merged

20 tasks

jiangpengcheng added 2 commits September 20, 2019 14:49

Add namespace field to activation log

e44705e

Add namespace field for ActivationFileStorage too

37ae815

jiangpengcheng force-pushed the add_namespace_field branch from 8b8b694 to 37ae815 Compare September 20, 2019 07:04

sven-lange-last reviewed Sep 20, 2019

View reviewed changes

jiangpengcheng added 2 commits September 23, 2019 17:23

Use activationToFileExtended

80a37a8

Apply fmt

08feb87

sven-lange-last approved these changes Sep 23, 2019

View reviewed changes

Apply scalaFmt

5ee2fc6

sven-lange-last approved these changes Sep 25, 2019

View reviewed changes

rabbah approved these changes Sep 26, 2019

View reviewed changes

sven-lange-last merged commit a581bea into apache:master Sep 26, 2019

BillZong pushed a commit to BillZong/openwhisk that referenced this pull request Nov 18, 2019

Add namespace field to activation log (apache#4609)

5126278

Add the activation namespace to all activation log lines forwarded to external logging solutions.

	val additionalFields = Map(config.userIdField -> context.user.namespace.uuid.toJson)

	activationFileStorage.activationToFile(activation, context, additionalFields)

	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)
	}

	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)

Conversation

jiangpengcheng commented Sep 6, 2019

Description

Related issue and scope

My changes affect the following components

Types of changes

Checklist:

Uh oh!

rabbah commented Sep 6, 2019

Uh oh!

jiangpengcheng commented Sep 6, 2019

Uh oh!

jiangpengcheng commented Sep 6, 2019

Uh oh!

sven-lange-last commented Sep 9, 2019

Uh oh!

codecov-io commented Sep 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

sven-lange-last commented Sep 9, 2019

Uh oh!

jiangpengcheng commented Sep 10, 2019

Uh oh!

rabbah left a comment

Choose a reason for hiding this comment

Uh oh!

jiangpengcheng commented Sep 16, 2019

Uh oh!

rabbah commented Sep 20, 2019

Uh oh!

jiangpengcheng commented Sep 20, 2019

Uh oh!

sven-lange-last commented Sep 20, 2019

Uh oh!

sven-lange-last Sep 20, 2019

Choose a reason for hiding this comment

Uh oh!

sven-lange-last Sep 20, 2019

Choose a reason for hiding this comment

Uh oh!

jiangpengcheng Sep 23, 2019

Choose a reason for hiding this comment

Uh oh!

sven-lange-last Sep 23, 2019

Choose a reason for hiding this comment

Uh oh!

jiangpengcheng Sep 23, 2019

Choose a reason for hiding this comment

Uh oh!

sven-lange-last Sep 23, 2019

Choose a reason for hiding this comment

Uh oh!

sven-lange-last left a comment

Choose a reason for hiding this comment

Uh oh!

jiangpengcheng commented Sep 24, 2019

Uh oh!

sven-lange-last left a comment

Choose a reason for hiding this comment

Uh oh!

jiangpengcheng commented Sep 25, 2019

Uh oh!

rabbah left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-io commented Sep 9, 2019 •

edited

Loading