Activation id in header #3671

style95 · 2018-05-18T08:42:34Z

This closes #3582

Description

This change will add activation id in the response headers.
Since same logic needs to be used in both collection API and web action API, I created dedicated trait for custom headers.

Related issue and scope

I opened an issue to propose and discuss this change ([Feature Request] Include activation ID in web action header. #3582)

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

Rather than add new tests, how about adding a header check to existing tests for invokes, and triggers.

rabbah · 2018-05-24T09:36:22Z

core/controller/src/main/scala/whisk/core/controller/Actions.scala

+
+        respondWithActivationIdHeader(activationId) {
+          // note that if header defined a content-type, it will be ignored
+          // since the type must be compatible with the data response


I don’t think this comment makes sense here.

rabbah · 2018-05-24T09:38:28Z

core/controller/src/main/scala/whisk/core/controller/WebActions.scala

@@ -634,37 +634,40 @@ trait WhiskWebActionsApi extends Directives with ValidateRequestSize with PostAc
                              responseType: MediaExtension)(implicit transid: TransactionId) = {
    onComplete(queuedActivation) {
      case Success(Right(activation)) =>
-        val result = activation.resultAsJson
+        respondWithActivationIdHeader(activation) {


I do not think the activation id should be reported in webaction. It leaks information. For normal POSTS it is ok because there’s an explicit subject key. For webactions, there is not.

This is to cope with the inconvenience which is described in #3582.
This will give users a better option to retrieve right activation results.

A webaction is called by an anonymous subject. The activation id is information specific to the subject owning the action.

The webaction can return its own header to identify the action; which could be the activation id if it chooses - I do not think the system should blanket return the activation id for webactions.

style95 · 2018-05-24T10:03:18Z

@rabbah
Activation id header checking is added in existing tests.

cbickel · 2018-05-24T13:34:09Z

core/controller/src/main/scala/whisk/core/controller/Entities.scala

+protected trait CustomHeaders extends Directives {
+
+  /** Add activation ID in headers */
+  protected def respondWithActivationIdHeader(activation: WhiskActivation): Directive0 =


Do we need to override this method?
Wouldn't it be easier to always pass the activationId? You have it anyway, if you have the activation.

Updated accordingly

cbickel · 2018-05-24T13:35:31Z

tests/src/test/scala/whisk/core/controller/test/ControllerTestCommon.scala

@@ -91,6 +91,8 @@ protected trait ControllerTestCommon
  val authStore = WhiskAuthStore.datastore()
  val logStore = SpiLoader.get[LogStoreProvider].logStore(actorSystem)

+  val ActivationIdHeaderInLowercase = "x-openwhisk-activation-id"


Can you move this variable to CustomHeaders and use it here as well? (And start with a lowercase letter)

Updated accordingly.

Regarding case, I followed scala constant naming convention(Upper camel case).
Is it OW convention(start with a lowercase letter)?

ddragosd · 2018-05-24T23:12:15Z

Given that the API Gateway sets the X-Request-Id as per #3199 , wouldn't it be easier to have the Gateway add it to the response header as well ?

rabbah · 2018-05-24T23:13:47Z

@ddragosd the request id isn't the activation id though --- although I raised the question in the related issue: should the two be the same in which case, the gateway or nginx generates the id and adds the header on the response.

ddragosd · 2018-05-24T23:15:04Z

should the two be the same

Any reason they can't be the same ?

rabbah · 2018-05-24T23:21:27Z

activation id needs to be unique to a namespace since it serves as part of the document id stored in the activation store (for metadata etc)... if that's satisfied i think we can do it.

ddragosd · 2018-05-24T23:37:10Z

makes sense @rabbah . I'd say we can safely assume that the Gateway generates unique activation IDs.

cbickel · 2018-05-25T09:51:04Z

@rabbah @ddragosd
I also thought about making tid and activationid the same, but I see the following problems:

If a trigger is invoked, you have a trigger activation and several action activations. Which of these activations should get the tid as activation id
Same for sequences. Which of the actions should get the tid as activation-id.
One of the ideas of sending the tid to the controller, was to generate the tid in one of the providers and pass it with the request. With this ability you would be able to trace a request from end to end. But this also adds the risk, that it could not be unique anymore.

But maybe there is an easy solution for all of these concerns ;)

codecov-io · 2018-05-25T09:51:45Z

Codecov Report

Merging #3671 into master will decrease coverage by 4.67%.
The diff coverage is 96.42%.

@@            Coverage Diff             @@
##           master    #3671      +/-   ##
==========================================
- Coverage   85.43%   80.75%   -4.68%     
==========================================
  Files         147      147              
  Lines        7070     7077       +7     
  Branches      423      423              
==========================================
- Hits         6040     5715     -325     
- Misses       1030     1362     +332

Impacted Files	Coverage Δ
...rc/main/scala/whisk/core/controller/Triggers.scala	`94.24% <100%> (+0.04%)`	⬆️
...rc/main/scala/whisk/core/controller/Entities.scala	`93.54% <100%> (+0.44%)`	⬆️
...src/main/scala/whisk/core/controller/Actions.scala	`92.96% <100%> (+0.07%)`	⬆️
.../main/scala/whisk/core/controller/WebActions.scala	`91.15% <92.85%> (+0.06%)`	⬆️
...core/database/cosmosdb/RxObservableImplicits.scala	`0% <0%> (-100%)`	⬇️
...core/database/cosmosdb/CosmosDBArtifactStore.scala	`0% <0%> (-95.1%)`	⬇️
...sk/core/database/cosmosdb/CosmosDBViewMapper.scala	`0% <0%> (-92.6%)`	⬇️
...whisk/core/database/cosmosdb/CosmosDBSupport.scala	`0% <0%> (-81.82%)`	⬇️
...abase/cosmosdb/CosmosDBArtifactStoreProvider.scala	`0% <0%> (-58.83%)`	⬇️
...la/whisk/core/database/cosmosdb/CosmosDBUtil.scala	`92% <0%> (-4%)`	⬇️
... and 1 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 e414b33...ec20733. Read the comment docs.

rabbah · 2018-05-25T10:11:12Z

For the first parts:

For a trigger, the request id is the triggers activation id. Every activated action generates a new activation id as we do today.

For a sequence the outermost action (the sequence itself) gets the id all the subcomponents get new ids (as today).

Same for conductor actions.

The uniqueness is the part I’m concerned about.

style95 · 2018-05-28T09:43:49Z

Let me recap the intermediate conclusion.

Activation ID should not be exposed by the system instead, users can decide to whether expose it or not using existing activation id parameter in case the action is a webaction.
Use request ID generated by nginx or gateway as Activation ID as well.

Regarding 1, I initially raised this issue in the aspect of UI. When providing the functionality to display activation results and logs after an invocation of an action in UI, it's not easy to select right activation when the action is webaction.

In case of normal action also, since activation logs are collected asynchronously, it's not that simple.
But anyway we can look it up because it returns activation ID in the response.

For webactions, it's not easy to choose a specific activation, and display logs for web actions without system support.

Any suggestions for this?

With regard to 2, so request ID would be Transaction ID and Activation ID.
More precisely Transaction id will be same with activation id. Would it be ok?

I just want to confirm.
As @ddragosd said if we can safely assume it is unique, would it be ok for this PR to take that in?

rabbah · 2018-06-11T04:59:17Z

@cbickel does #3671 (comment) address your concerns?

if we adopt the nginx generated id as the activation id, then this pr becomes a lot simpler.
we can also remove the double headers currently generated by the controller (and only emit it if an id isn't received).

cbickel · 2018-06-11T08:43:53Z

@rabbah
Not completely. I agree, that this would be the best solution, but the problem is, that on firing a trigger, the controller is calling itself with the transaction-Id.
To my knowledge, the controller doesn't recognise today, that the action is part of a rule or a normal action. Or am I wrong?
And the uniqueness is still a problem. If the tids are not unique, it would be bad for debugging, but if activation-Ids are not unique anymore, users wont't get activation-records anymore.

markusthoemmes · 2018-07-23T10:04:03Z

tests/src/test/scala/whisk/core/controller/test/ActionsApiTests.scala

@@ -1043,13 +1043,15 @@ class ActionsApiTests extends ControllerTestCommon with WhiskActionsApi {
      status should be(Accepted)
      val response = responseAs[JsObject]
      response.fields("activationId") should not be None
+      headers.exists(_.is(ActivationIdHeaderInLowercase)) should be(true)


headers are immutable and per the line above you know the correct activation id. You should be able to rewrite this to something like:

headers should contain(RawHeader(ActivationIdHeader, response.fields("activationId").convertTo[String]))

That'll give you a more explanatory response in tests. WDYT?

markusthoemmes · 2018-07-23T10:04:45Z

tests/src/test/scala/whisk/core/controller/test/WebActionsApiTests.scala

+              headers.exists(header => {
+                header.is("set-cookie") && header.value() == "a=b"
+              }) shouldBe true
+              headers.exists(_.is(ActivationIdHeaderInLowercase)) shouldBe true


See comment above, please rewrite both of these into "contains" checks.

markusthoemmes · 2018-07-23T10:04:59Z

tests/src/test/scala/whisk/core/controller/test/ActionsApiTests.scala

      }

      // repeat invoke, get only result back
      Post(s"$collectionPath/${action.name}?blocking=true&result=true") ~> Route.seal(routes(creds)) ~> check {
+        headers


markusthoemmes · 2018-07-23T10:06:35Z

Is there something actionable in here? Should we take the discussion out to the dev-list? At least the bit on generating activationIds from something outside of the controller seems worth discussing in a broader audience.

style95 · 2018-08-08T11:40:09Z

I think the point is, if we allow activationID generation in multiple components(nginx, controller), we cannot guarantee the activationID is unique(though the possibility of duplicate activationID is less.)

To use the same ID for TransactionID and ActivationID, it should be generated in nginx.
But also, ActivationID should be generated in controllers as a trigger is fired by controllers.

style95 · 2018-08-08T11:42:38Z

@rabbah
Regarding exposure of ActivationID of webaction in response header, how about having a configuration to control it by OW operator?
In some cases, it would be worth to expose it though it could be a kind of small data breaches.

rabbah · 2018-08-08T11:57:12Z

I think I've changed perspective on the activation id in the header response of web actions, after some further consideration (to agree with you, and that it's OK). I can explain in a longer comment later if necessary.

style95 · 2018-08-14T09:42:48Z

How about separating ActivationID addition in headers and using same ID for TransactionID and ActivationID?
I think there is not big correleation between the two.

How about taking these changes and discussing the second one in dev-list or another PR separately?

rabbah · 2018-08-14T10:21:18Z

tests/src/test/scala/whisk/core/controller/test/ActionsApiTests.scala

      }

      // repeat this time wait longer than active ack delay
      Post(s"$collectionPath/${action.name}?blocking=true&timeout=500") ~> Route.seal(routes(creds)) ~> check {
        status shouldBe OK
        val response = responseAs[JsObject]
        response shouldBe activation.withoutLogs.toExtendedJson
+        headers should contain(RawHeader(ActivationIdHeader, response.fields("activationId").convertTo[String]))
+


Extra line.

rabbah · 2018-08-14T10:21:28Z

tests/src/test/scala/whisk/core/controller/test/ActionsApiTests.scala

@@ -1319,6 +1329,8 @@ class ActionsApiTests extends ControllerTestCommon with WhiskActionsApi {
        status should be(InternalServerError)
        val response = responseAs[JsObject]
        response should be(activation.withoutLogs.toExtendedJson)
+        headers should contain(RawHeader(ActivationIdHeader, response.fields("activationId").convertTo[String]))
+


Extra line.

rabbah · 2018-08-14T10:22:14Z

tests/src/test/scala/whisk/core/controller/test/ControllerTestCommon.scala

@@ -89,6 +89,8 @@ protected trait ControllerTestCommon
  val logStore = SpiLoader.get[LogStoreProvider].instance(actorSystem)
  val activationStore = SpiLoader.get[ActivationStoreProvider].instance(actorSystem, materializer, logging)

+  val ActivationIdHeaderInLowercase = ActivationIdHeader.toLowerCase
+


Is this used?

I think it would be better to keep it because I restored the previous exist logic.
Unlike a normal action, we are unable to get the value of ActivationID from the response as so unable to use contain logic that Markus suggested.

headers should contain(RawHeader(ActivationIdHeader, _)) // does not work

I'd propose to make the global setting lowercased then, and use that throughout. WDYT?

Sorry, what is the global setting?

The value in CustomHeaders?

style95 · 2018-08-16T03:30:36Z

Travis build has passed.

style95 · 2018-08-17T07:04:04Z

Updated accordingly

rabbah · 2018-08-17T11:47:30Z

core/controller/src/main/scala/whisk/core/controller/Entities.scala

@@ -65,15 +65,24 @@ protected[controller] trait ValidateRequestSize extends Directives {
  protected val fieldDescriptionForSizeError = "Request"
 }

+protected trait CustomHeaders extends Directives {
+  val ActivationIdHeader = "x-openwhisk-activation-id"


I think @mhenke1 introduced a header with X-OW as the prefix. API gateway uses x-openwhisk. We should normalize and collocate these. Fwiw nginx also generates x-request-id header.

@rabbah Agree.
How about using x-openwhisk?
Since x- header is a custom header specific to the application, it would be better to use the detailed one.

I am not sure changes to use the same OpenWhisk prefix for all custom headers should be included in this PR.

style95 · 2018-08-22T10:48:09Z

Any comments on this?

rabbah · 2018-08-22T18:42:10Z

I think we should still pursue id consolidation - I don't think we need both an activation id and a transaction id for top level activations, and we should further then reduce the number of headers we're communicating back (it would just be one id that means both). Deferring to another discussion/PR.

rabbah · 2019-01-11T04:02:31Z

I used this today. 💪

style95 · 2019-01-11T05:20:49Z

Sounds great to me :)

style95 closed this May 23, 2018

style95 reopened this May 23, 2018

markusthoemmes requested a review from cbickel May 23, 2018 07:55

markusthoemmes assigned cbickel May 23, 2018

rabbah reviewed May 24, 2018

View reviewed changes

style95 force-pushed the activation-id-in-header branch from e12a51e to 10e3028 Compare May 24, 2018 10:01

cbickel reviewed May 24, 2018

View reviewed changes

style95 force-pushed the activation-id-in-header branch from 10e3028 to ae4f98f Compare May 25, 2018 09:18

markusthoemmes reviewed Jul 23, 2018

View reviewed changes

style95 force-pushed the activation-id-in-header branch from ae4f98f to 1ce7b2f Compare August 14, 2018 09:49

style95 force-pushed the activation-id-in-header branch from 1ce7b2f to 187992d Compare August 14, 2018 09:58

rabbah approved these changes Aug 14, 2018

View reviewed changes

style95 force-pushed the activation-id-in-header branch 2 times, most recently from 6d83f8d to 8d9608e Compare August 16, 2018 02:09

style95 force-pushed the activation-id-in-header branch from 152c29f to 19e64fc Compare August 17, 2018 03:49

style95 closed this Aug 17, 2018

style95 reopened this Aug 17, 2018

rabbah reviewed Aug 17, 2018

View reviewed changes

style95 added 2 commits August 22, 2018 17:32

Apply review comments

8a393f2

Remove extra lines

5e313d1

style95 force-pushed the activation-id-in-header branch from 19e64fc to ec20733 Compare August 22, 2018 08:35

style95 added 2 commits August 22, 2018 17:36

Fix test failure

46a8c10

Make ActivatioId value lowercased.

ec20733

rabbah approved these changes Aug 22, 2018

View reviewed changes

rabbah merged commit 75472f8 into apache:master Aug 22, 2018

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

Add the activation id in the HTTP header response (apache#3671)

3d1f175

Activation id in header #3671

Activation id in header #3671

Conversation

style95 commented May 18, 2018 • edited

Description

Related issue and scope

My changes affect the following components

Types of changes

Checklist:

rabbah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rabbah May 24, 2018 • edited

Choose a reason for hiding this comment

style95 commented May 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ddragosd commented May 24, 2018

rabbah commented May 24, 2018

ddragosd commented May 24, 2018

rabbah commented May 24, 2018

ddragosd commented May 24, 2018

cbickel commented May 25, 2018

codecov-io commented May 25, 2018 • edited

Codecov Report

rabbah commented May 25, 2018

style95 commented May 28, 2018 • edited

rabbah commented Jun 11, 2018

cbickel commented Jun 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markusthoemmes commented Jul 23, 2018

style95 commented Aug 8, 2018

style95 commented Aug 8, 2018

rabbah commented Aug 8, 2018 • edited

style95 commented Aug 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

style95 Aug 16, 2018 • edited

Choose a reason for hiding this comment

style95 Aug 16, 2018 • edited

Choose a reason for hiding this comment

style95 commented Aug 16, 2018

style95 commented Aug 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

style95 commented Aug 22, 2018

rabbah commented Aug 22, 2018

rabbah commented Jan 11, 2019

style95 commented Jan 11, 2019

style95 commented May 18, 2018 •

edited

rabbah May 24, 2018 •

edited

codecov-io commented May 25, 2018 •

edited

style95 commented May 28, 2018 •

edited

rabbah commented Aug 8, 2018 •

edited

style95 Aug 16, 2018 •

edited

style95 Aug 16, 2018 •

edited