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

new: add ability to identify which PushConfig an Event is for #87

Closed
wants to merge 6 commits into from

Conversation

@aaslamin
Copy link
Contributor

aaslamin commented Feb 7, 2020

⚠️ Dependencies (must be merged first):

All the PR's in this project: https://github.com/orgs/aporeto-inc/projects/759
aporeto-inc/elemental#66


This PR addresses aporeto-inc/aporeto#2490 by adding the elemental PushConfig's identifier to events if the client configured had configured an identifier on their PushConfig message.

@aaslamin aaslamin requested review from primalmotion and t00f Feb 7, 2020
@aaslamin aaslamin force-pushed the bahamut/identify-events branch from d8bd379 to 1adfeaa Feb 7, 2020
@@ -23,6 +23,8 @@ require (
github.com/prometheus/client_golang v1.0.0
github.com/smartystreets/assertions v1.0.1 // indirect
github.com/smartystreets/goconvey v0.0.0-20190731233626-505e41936337
github.com/tidwall/gjson v1.4.0 // indirect
github.com/tidwall/sjson v1.0.4

This comment has been minimized.

Copy link
@aaslamin

aaslamin Feb 7, 2020

Author Contributor

@primalmotion @t00f ℹ️This package allows us to CRUD raw JSON efficiently without having to incur the penalty of (un)marshaling again.

This is useful for injecting the push configuration ID after having encoding the event once for all clients.

See benchmarks


// avoid compiler optimizations by assigning result to a package level var to avoid ruining the benchmark.
result = []byte(tmp)
}

This comment has been minimized.

Copy link
@aaslamin

aaslamin Feb 7, 2020

Author Contributor

@t00f @primalmotion benchmarks are quite impressive

go test -run=XXX -bench=. -benchtime=5s -v
goos: darwin
goarch: amd64
pkg: go.aporeto.io/bahamut
BenchmarkDecodeEncodeJSON_AddPushConfigIDForEvent-12          	  811402	      6719 ns/op	     889 B/op	      11 allocs/op
BenchmarkDecodeEncodeJSON_AddPushConfigIDForEvent_SJSON-12    	 8657688	       746 ns/op	     888 B/op	       6 allocs/op
PASS
ok  	go.aporeto.io/bahamut	13.164s
@aaslamin aaslamin marked this pull request as ready for review Feb 8, 2020
switch session.encodingWrite {
case elemental.EncodingTypeMSGPACK:
session.send(dataMSGPACK)
data := dataMSGPACK

This comment has been minimized.

Copy link
@aaslamin

aaslamin Feb 10, 2020

Author Contributor

ℹ️ You must create a copy otherwise you would be adding someone else's ID to all events that are being pushed out.

This comment has been minimized.

Copy link
@primalmotion

primalmotion Mar 2, 2020

Member

all of this is deoptimizing a lot's of things. Can we please find another way?
What is the goal of sending the pushConfigID already? If it's just for UI handling sake, I would rather de-optimize there than in the backend push system

@aaslamin

This comment has been minimized.

Copy link
Contributor Author

aaslamin commented Feb 12, 2020

/build - automatically fired by gogo with following PRs and commit SHAs v1.0.0

[
  {
    "project": "ws-event-identity",
    "component": "bahamut",
    "pr-id": "87",
    "commit-sha": "b0d690a5b69f29196169246727f414dfe66d1f4c",
    "pipeline": "master"
  },
  {
    "project": "ws-event-identity",
    "component": "elemental",
    "pr-id": "66",
    "commit-sha": "f4a67f1483a627ed073fd5b438ded4dfadbdd523",
    "pipeline": "master"
  },
  {
    "project": "ws-event-identity",
    "component": "backend",
    "pr-id": "677",
    "commit-sha": "3265d4afda3cfda9a9e84bc24da2034bf1096f66",
    "pipeline": "master"
  }
]
@aaslamin aaslamin force-pushed the bahamut/support-error-events branch from 3ae1143 to 439450b Feb 25, 2020
@aaslamin aaslamin force-pushed the bahamut/identify-events branch from b0d690a to 4471d04 Feb 26, 2020
@aaslamin

This comment has been minimized.

Copy link
Contributor Author

aaslamin commented Feb 26, 2020

Update: rebased w/ the latest changes add to branch bahamut/support-error-events

@aaslamin aaslamin changed the base branch from bahamut/support-error-events to master Feb 27, 2020
@aaslamin aaslamin force-pushed the bahamut/identify-events branch from 4471d04 to c2c8c38 Feb 27, 2020
aaslamin added 6 commits Feb 7, 2020
…onfig's identifier to events if the client configured had configured an identifier on their PushConfig message.
…l/sjson package for CRUDing raw JSON bytes
Given a session has set the push config ID, events that are pushed for that session should contain the ID - MSGPACK
@aaslamin aaslamin force-pushed the bahamut/identify-events branch from c2c8c38 to aff2e78 Feb 27, 2020
@aaslamin

This comment has been minimized.

Copy link
Contributor Author

aaslamin commented Feb 27, 2020

/build - automatically fired by gogo with following PRs and commit SHAs v1.0.0

[
  {
    "project": "ws-event-identity",
    "component": "bahamut",
    "pr-id": "87",
    "commit-sha": "aff2e78482e8d78ec3848140ef0c17b4c6533c84",
    "pipeline": "master"
  },
  {
    "project": "ws-event-identity",
    "component": "elemental",
    "pr-id": "66",
    "commit-sha": "8998e1dadc3bb074a3503bd0b63f6550dd7f62de",
    "pipeline": "master"
  },
  {
    "project": "ws-event-identity",
    "component": "backend",
    "pr-id": "695",
    "commit-sha": "b27644e2692b1c44de05d90c1b5b514a153b59ea",
    "pipeline": "master"
  }
]
1 similar comment
@aaslamin

This comment has been minimized.

Copy link
Contributor Author

aaslamin commented Feb 27, 2020

/build - automatically fired by gogo with following PRs and commit SHAs v1.0.0

[
  {
    "project": "ws-event-identity",
    "component": "bahamut",
    "pr-id": "87",
    "commit-sha": "aff2e78482e8d78ec3848140ef0c17b4c6533c84",
    "pipeline": "master"
  },
  {
    "project": "ws-event-identity",
    "component": "elemental",
    "pr-id": "66",
    "commit-sha": "8998e1dadc3bb074a3503bd0b63f6550dd7f62de",
    "pipeline": "master"
  },
  {
    "project": "ws-event-identity",
    "component": "backend",
    "pr-id": "695",
    "commit-sha": "b27644e2692b1c44de05d90c1b5b514a153b59ea",
    "pipeline": "master"
  }
]
@aaslamin

This comment has been minimized.

Copy link
Contributor Author

aaslamin commented Feb 27, 2020

/build - automatically fired by gogo with following PRs and commit SHAs v1.0.0

[
  {
    "project": "ws-event-identity",
    "component": "bahamut",
    "pr-id": "87",
    "commit-sha": "aff2e78482e8d78ec3848140ef0c17b4c6533c84",
    "pipeline": "master"
  },
  {
    "project": "ws-event-identity",
    "component": "elemental",
    "pr-id": "66",
    "commit-sha": "8998e1dadc3bb074a3503bd0b63f6550dd7f62de",
    "pipeline": "master"
  },
  {
    "project": "ws-event-identity",
    "component": "backend",
    "pr-id": "695",
    "commit-sha": "2f9686c160458da42c2b3bb90d1d82914530b411",
    "pipeline": "master"
  }
]
@aaslamin

This comment has been minimized.

Copy link
Contributor Author

aaslamin commented Feb 27, 2020

CI is failing to compile because this PR depends on aporeto-inc/elemental#66

See the backend integration test for confirmation that this works A-OK 👌

defer s.currentPushConfigLock.RUnlock()

if s.pushConfig == nil || s.pushConfig.ID == "" {
return "", false

This comment has been minimized.

Copy link
@primalmotion

primalmotion Mar 2, 2020

Member

what's the use of the returned boolean here? It looks pretty useless

This comment has been minimized.

Copy link
@aaslamin

aaslamin Mar 2, 2020

Author Contributor

It's to indicate that a push configuration has been provided by the client. Otherwise, you would be doing a check on the empty string which is the default value for an uninitialized string.

switch session.encodingWrite {
case elemental.EncodingTypeMSGPACK:
session.send(dataMSGPACK)
data := dataMSGPACK

This comment has been minimized.

Copy link
@primalmotion

primalmotion Mar 2, 2020

Member

all of this is deoptimizing a lot's of things. Can we please find another way?
What is the goal of sending the pushConfigID already? If it's just for UI handling sake, I would rather de-optimize there than in the backend push system

@aaslamin

This comment has been minimized.

Copy link
Contributor Author

aaslamin commented Mar 11, 2020

Closing since this has a performance penalty we cannot incur. We will come up with something else!

@aaslamin aaslamin closed this Mar 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.