openzipkin / zipkin-aws

Reporters and collectors for use in Amazon's cloud
Apache License 2.0
69 stars 34 forks source link

Kinesis collector does not use Region in Docker #97

Closed freelanceadm closed 6 years ago

freelanceadm commented 6 years ago

Sorry if I make issue in wrong format. I am running zipkin with kinesis collector. My kinesis stream is us-east-2 but Kinesis collector always try to connect to us-east-1. When Kinesis stream in us-east-1 all is working fine. And here is my docker-compose.yml

version: '2' services: zipkin: image: openzipkin/zipkin-aws container_name: zipkin-aws ports:


My region is us-east-2 but Kinesis collector alway try to connect to us-east-1.

zipkin-aws | 2018-07-20 10:34:34.799 INFO 5 --- [ main] o.s.j.e.a.AnnotationMBeanExporter : Registering beans for JMX exposure on startup zipkin-aws | 2018-07-20 10:34:35.258 INFO 5 --- [ main] o.s.b.w.e.u.UndertowServletWebServer : Undertow started on port(s) 9411 (http) with context path '' zipkin-aws | 2018-07-20 10:34:35.277 INFO 5 --- [ main] z.s.ZipkinServer : Started ZipkinServer in 19.063 seconds (JVM running for 20.253) zipkin-aws | 2018-07-20 10:34:35.468 INFO 5 --- [inesis-zipkin-0] c.a.s.k.c.l.w.Worker : Syncing Kinesis shard info zipkin-aws | 2018-07-20 10:34:35.709 ERROR 5 --- [inesis-zipkin-0] c.a.s.k.c.l.w.ShardSyncTask : Caught exception while sync'ing Kinesis shards and leases zipkin-aws | zipkin-aws | com.amazonaws.services.kinesis.model.ResourceNotFoundException: Stream kinesis-zipkin under account not found. (Service: AmazonKinesis; Status Code: 400; Error Code: ResourceNotFoundException; Request ID: ****) zipkin-aws | at com.amazonaws.http.AmazonHttpClient$RequestExecutor.handleErrorResponse(AmazonHttpClient.java:1640) ~[aws-java-sdk-core-1.11.348.jar!/:?] zipkin-aws | at com.amazonaws.http.AmazonHttpClient$RequestExecutor.executeOneRequest(AmazonHttpClient.java:1304) ~[aws-java-sdk-core-1.11.348.jar!/:?] zipkin-aws | at com.amazonaws.http.AmazonHttpClient$RequestExecutor.executeHelper(AmazonHttpClient.java:1058) ~[aws-java-sdk-core-1.11.348.jar!/:?] zipkin-aws | at com.amazonaws.http.AmazonHttpClient$RequestExecutor.doExecute(AmazonHttpClient.java:743) ~[aws-java-sdk-core-1.11.348.jar!/:?] zipkin-aws | at com.amazonaws.http.AmazonHttpClient$RequestExecutor.executeWithTimer(AmazonHttpClient.java:717) ~[aws-java-sdk-core-1.11.348.jar!/:?] zipkin-aws | at com.amazonaws.http.AmazonHttpClient$RequestExecutor.execute(AmazonHttpClient.java:699) ~[aws-java-sdk-core-1.11.348.jar!/:?] zipkin-aws | at com.amazonaws.http.AmazonHttpClient$RequestExecutor.access$500(AmazonHttpClient.java:667) ~[aws-java-sdk-core-1.11.348.jar!/:?] zipkin-aws | at com.amazonaws.http.AmazonHttpClient$RequestExecutionBuilderImpl.execute(AmazonHttpClient.java:649) ~[aws-java-sdk-core-1.11.348.jar!/:?] zipkin-aws | at com.amazonaws.http.AmazonHttpClient.execute(AmazonHttpClient.java:513) ~[aws-java-sdk-core-1.11.348.jar!/:?] zipkin-aws | at com.amazonaws.services.kinesis.AmazonKinesisClient.doInvoke(AmazonKinesisClient.java:2388) ~[aws-java-sdk-kinesis-1.11.348.jar!/:?] zipkin-aws | at com.amazonaws.services.kinesis.AmazonKinesisClient.invoke(AmazonKinesisClient.java:2364) ~[aws-java-sdk-kinesis-1.11.348.jar!/:?] zipkin-aws | at com.amazonaws.services.kinesis.AmazonKinesisClient.executeListShards(AmazonKinesisClient.java:1337) ~[aws-java-sdk-kinesis-1.11.348.jar!/:?] zipkin-aws | at com.amazonaws.services.kinesis.AmazonKinesisClient.listShards(AmazonKinesisClient.java:1312) ~[aws-java-sdk-kinesis-1.11.348.jar!/:?] zipkin-aws | at com.amazonaws.services.kinesis.clientlibrary.proxies.KinesisProxy.listShards(KinesisProxy.java:304) ~[amazon-kinesis-client-1.9.1.jar!/:?] zipkin-aws | at com.amazonaws.services.kinesis.clientlibrary.proxies.KinesisProxy.getShardList(KinesisProxy.java:365) ~[amazon-kinesis-client-1.9.1.jar!/:?] zipkin-aws | at com.amazonaws.services.kinesis.clientlibrary.lib.worker.ShardSyncer.getShardList(ShardSyncer.java:319) ~[amazon-kinesis-client-1.9.1.jar!/:?] zipkin-aws | at com.amazonaws.services.kinesis.clientlibrary.lib.worker.ShardSyncer.syncShardLeases(ShardSyncer.java:121) ~[amazon-kinesis-client-1.9.1.jar!/:?] zipkin-aws | at com.amazonaws.services.kinesis.clientlibrary.lib.worker.ShardSyncer.checkAndCreateLeasesForNewShards(ShardSyncer.java:90) ~[amazon-kinesis-client-1.9.1.jar!/:?] zipkin-aws | at com.amazonaws.services.kinesis.clientlibrary.lib.worker.ShardSyncTask.call(ShardSyncTask.java:71) [amazon-kinesis-client-1.9.1.jar!/:?] zipkin-aws | at com.amazonaws.services.kinesis.clientlibrary.lib.worker.MetricsCollectingTaskDecorator.call(MetricsCollectingTaskDecorator.java:49) [amazon-kinesis-client-1.9.1.jar!/:?] zipkin-aws | at com.amazonaws.services.kinesis.clientlibrary.lib.worker.Worker.initialize(Worker.java:635) [amazon-kinesis-client-1.9.1.jar!/:?] zipkin-aws | at com.amazonaws.services.kinesis.clientlibrary.lib.worker.Worker.run(Worker.java:566) [amazon-kinesis-client-1.9.1.jar!/:?] zipkin-aws | at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149) [?:1.8.0_171] zipkin-aws | at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624) [?:1.8.0_171] zipkin-aws | at java.lang.Thread.run(Thread.java:748) [?:1.8.0_171]

devinsba commented 6 years ago

Looks like we have to augment our use of the Kinesis client with:

https://github.com/awslabs/amazon-kinesis-client/blob/master/src/main/java/com/amazonaws/services/kinesis/clientlibrary/lib/worker/KinesisClientLibConfiguration.java#L1208

Which we would have to use here:

https://github.com/openzipkin/zipkin-aws/blob/master/collector-kinesis/src/main/java/zipkin2/collector/kinesis/KinesisCollector.java#L121

jtanza commented 6 years ago

Hey, I've never contributed to the repo before, but I would like to pick this one up, that alright?

devinsba commented 6 years ago

@jtanza absolutely!

jtanza commented 6 years ago

awesome, i'll take a crack at it then. The approach you've outlined above seems pretty straightforward so ill start there :)

freelanceadm commented 6 years ago

Also I see that kinesis collector use only us-east-1 region. To manage access from several containers to Kinesis it use DynamoDB tables and it also create such tables in us-east-1.

jtanza commented 6 years ago

So as @devinsba alluded to above, it seems the issue is due to the lack of configuration options for setting different aws regions for a user's registered kinesis stream. In 3613e5 I've added the option to set the region directly within the zipkin-server-kinesis.yml config.

I've tested things locally on my machine with the zipkin-server executable and the collector-kinesis autoconfigure jar from the new build, and was able to connect to a kinesis stream in us-west-2 (Oregon) without issue (previously trying to connect with a kinesis stream outisde of us-east-1 would throw a com.amazonaws.services.kinesis.model.ResourceNotFoundException: Stream zipkin under account 198xxxxxx not found (Service: AmazonKinesis; Status Code: 400; Error Code: ResourceNotFoundException;))

So, the only thing I am unsure of at this point is if additional code changes are required to make my changes accessible to the docker image via the docker-compose.yml @freelanceadm mentioned?

I am happy to create a PR if those are the next steps, but I am not sure if any testing is required beforehand (all unit tests are passing on my end) or if a PR is required to test. Just let me know!

codefromthecrypt commented 6 years ago

made some notes on your commit. Sort those and you'll notice unit tests for auto-config. It is likely sensible to have the STS region set implicitly to the same as the kinesis by default. So that you don't have to set the same thing twice, unless I'm missing a case where it is extremely common to have these in two regions and they aren't likely to be the same.

On Thu, Aug 2, 2018 at 6:39 AM, john tanza notifications@github.com wrote:

So as @devinsba https://github.com/devinsba alluded to above, it seems the issue is due to the lack of configuration options for setting different aws regions for the registered kinesis stream. In 3613e5 https://github.com/jtanza/zipkin-aws/commit/3613e53ec13bb00e6f991a462fcc91f3de467b22 I've added the option to set the region directly within the zipkin-server-kinesis.yml https://github.com/jtanza/zipkin-aws/commit/3613e53ec13bb00e6f991a462fcc91f3de467b22#diff-cfeb63b4b189f56705dff657d59b5892 config. I've tested things locally on my machine with the zipkin-server executable and the collector-kinesis autoconfigure jar from the new build, and was able to connect to a kinesis stream in us-west-2 (Oregon) without issue (previously trying to connect with a kinesis stream outisde of us-east-1 would throw a com.amazonaws.services.kinesis.model.ResourceNotFoundException: Stream zipkin under account 198xxxxxx not found (Service: AmazonKinesis; Status Code: 400; Error Code: ResourceNotFoundException;))

So, the only thing I am unsure of at this point is if additional code changes are required to make my changes accessible to the docker image via the docker-compose.yml @freelanceadm https://github.com/freelanceadm mentioned?

I am happy to create a PR if those are the next steps, but I am not sure if any testing is required beforehand (all unit tests are passing on my end). Just let me know!

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/openzipkin/zipkin-aws/issues/97#issuecomment-409748037, or mute the thread https://github.com/notifications/unsubscribe-auth/AAD614P-9sNY5u7_1ga31LtPb5ow-9hgks5uMi4bgaJpZM4VX1t0 .

jtanza commented 6 years ago

Okay all set, comments addressed. One point of confusion however, are you suggesting we remove the KINESIS_AWS_STS_REGION field from zipkin-server-kinesis.yml entirely and use the value provided in AWS_REGION for both (sts and the kinesis stream)?

alexandr-pomeshkin-aval commented 6 years ago

I suggest to add and use AWS_REGION variable for Zipkin application to setup region there it must work/serch for Kinesis/SQS other services.

codefromthecrypt commented 6 years ago

are you planning to use SQS and Kinesis at the same time? I understand this sounds intuitive, but on the other hand each of these can be present without eachother in a custom build.

codefromthecrypt commented 6 years ago

So, what I mean is that all of our plugins are self-contained.. they don't have variables defined outside their namespaces, and don't depend on some "aws-autoconfiguration-core" or such which could lead to some versioning complexity. I'd advocate for leaving things namespaced inside the services for now.

Particular to this change, the only thing asked was if it was ever sensible to have STS as a different region than the Kinesis one. Even if it was, we can default one to the other. For example KINESIS_AWS_REGION implicitly sets KINESIS_AWS_STS_REGION. I can help add this code if how to achieve this is unclear.

freelanceadm commented 6 years ago

The probles is that Zipkin use only default region - us-east-1. Here is my pipeline: us-east-2 applications send traces to -> kinesis -> Zipkin get traces -> Send data to Elasticsearch

Here is issues: 1 Need to deploy Kinesis stream only in us-east-1 2 Applications need to send data cross region only to Kinesis streams in us-east-1 3 Kinesis can use Elasticsearch only in us-east-1 Thats why I suggest to add variable to Zipkin base image/code AWS_REGION. And use this variable when Zipkin lookup for resources. Setup/change code for plugins to use AWS_REGION provided from Zipkin. This will remove complexity. Most deployments are in the same region, thats why its unusual for applications to work in one region and get data from the other. Usually they have one variable. If there is a lot of users who need cross-region then plugins can use their own environment variables its defined: for example SQS_AWS_REGION, KINESIS_AWS_REGION and etc.

codefromthecrypt commented 6 years ago

I'm getting the sense no one will answer about STS being valid or not in another region as the primary service, so will move on.

About introducing another variable AWS_REGION, this is another change. first we need to fix this one. That other change would modify all the other things.

codefromthecrypt commented 6 years ago

OK, so here's the plan.

make sure the things that implicitly use STS (the security token service) inherit the STS region (ex. KINESIS_AWS_STS_REGION) from the service specific one (ex KINESIS_AWS_REGION)

then, make sure the service specific region (ex KINESIS_AWS_REGION), if not set goes to an AWS_REGION variable, if that is set.

This should sort out the issues raised without breaking existing env variables. Some config duplication should be possible to prevent some coupling as well.

freelanceadm commented 6 years ago

Yes that's right I think this would be the right way.

вт, 7 авг. 2018 г., 11:59 Adrian Cole notifications@github.com:

OK, so here's the plan.

make sure the things that implicitly use STS (the security token service) inherit the STS region (ex. KINESIS_AWS_STS_REGION) from the service specific one (ex KINESIS_AWS_REGION)

then, make sure the service specific region (ex KINESIS_AWS_REGION), if not set goes to an AWS_REGION variable, if that is set.

This should sort out the issues raised without breaking existing env variables. Some config duplication should be possible to prevent some coupling as well.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openzipkin/zipkin-aws/issues/97#issuecomment-410986214, or mute the thread https://github.com/notifications/unsubscribe-auth/AFc92n_q4UIEG5B8jUfMrasqGrekup3eks5uOVb-gaJpZM4VX1t0 .

jtanza commented 6 years ago

Cool. I've gone ahead and implemented in 6a1406c what I hope was the correct interpretation of what you suggested @adriancole . If anything is off there, I'd be happy to fix/ welcome you to make any changes directly on the branch.

codefromthecrypt commented 6 years ago

I made one comment on that.. If we go for this, then we should do the same with the other autconfiguration classes, so that the top-level readme can mention the generality (which will help when people are using multiple services) On Wed, Aug 8, 2018 at 8:34 AM john tanza notifications@github.com wrote:

Cool. I've gone ahead and implemented in 6a1406c what I hope was the correct interpretation of what you suggested @adriancole . If anything is off there, I'd be happy to fix/ welcome you to make any changes directly on the branch.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

jtanza commented 6 years ago

Okie doke, plan is to add the variable nesting to the Kinesis config as mentioned in the recent commit comments.

Apart from that, It seems the only other autoconfigure service that would benefit from adding the AWS_REGION and other region configuration logic is the collector-sqs package? If that's correct, I'll go ahead and make those changes.

codefromthecrypt commented 6 years ago

I think so yes. unless there is some parity concerns with elasticsearch.

jtanza commented 6 years ago

Alrighty, after a bit more digging, turns out explicitly setting the AWS region for SQS as it was done for Kinesis isn't necessary and moreover isn't possible (see AwsClientBuilder). However, this really shouldn't be an issue as the region can be set directly via EndpointConfiguration in the ZipkinSQSCollectorAutoConfiguration; this should already be the case, however I failed to find this statement in the code base directly. Perhaps adding a line about this in the SQS readme would be helpful?

In any event, in 9f614ef I've gone ahead and made the requisite changes to Kinesis config mentioned above.

codefromthecrypt commented 6 years ago

great. can you check for parity with storage-elasticsearch-aws?

jtanza commented 6 years ago

oh woops, forgot to mention that, no parity issue with elastisearch as far as I can see: no STS options there, and region setting is accomplished via ES_AWS_REGION from what I can see.

codefromthecrypt commented 6 years ago

ok great. then go ahead and pull request and assuming tests are there and README if relevant is updated, should be an easy merge

devinsba commented 6 years ago

closed by #104