-
Notifications
You must be signed in to change notification settings - Fork 91
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
create http dispatcher #91
base: main
Are you sure you want to change the base?
Conversation
datastore/http/http.go
Outdated
namespace: namespace, | ||
metricsCollector: metricsCollector, | ||
records: requests, | ||
address: config.Address, |
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.
can address be localhost? We should prevent that in validateConfig
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.
Why do you say we should prevent localhost?
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 that not mean http producer can call fleet telemetry server itself?
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.
The producer could call fleet telemetry server but I believe that would be rejected due to invalid certificates. And it would be useless.
The use case I am picturing is a server running with these processes:
- fleet-telemetry: exposed on port 4443
- custom-web-server: internally on port 3000. This is what receives vehicle data over http.
bfeb4d8
to
04e67b7
Compare
04e67b7
to
9d8cc25
Compare
datastore/simple/logger_test.go
Outdated
@@ -14,51 +15,28 @@ import ( | |||
var _ = Describe("Proto Logger", func() { | |||
var ( | |||
protoLogger *simple.ProtoLogger | |||
logger *logrus.Logger |
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.
do we need logger 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.
Revised. This section is a bit complicated due to BeforeEach
needing to define an outer and local scope variable from a function that returns multiple values.
9d8cc25
to
bbb9b89
Compare
bbb9b89
to
a887fa7
Compare
@agbpatro / @aaronpkahn could I get eyes on this again? |
I am still evaluating the value proposition for it. We already have support ZMQ for which small scale developers can use for their application without infra overhead and can do same things which http producer implemented here does |
Hey @agbpatro ! I would really like to see this feature, it is a great idea, and provides a simple and easy solution for integration and PoC codes. |
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.
@patrickdemers6 can you rebase the PR so that it picks up latest changes and resolve merge conflicts?
a887fa7
to
a327de1
Compare
datastore/http/http.go
Outdated
|
||
func (p *Producer) sendHTTP(record *telemetry.Record) { | ||
url := fmt.Sprintf("%s?namespace=%s&type=%s", p.address, p.namespace, record.TxType) | ||
payloadJSON, err := record.GetJSONPayload() |
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.
change this to record.Payload()
to make it consistent with other producers? We have a top level support to send data in json https://github.com/teslamotors/fleet-telemetry/blob/main/README.md#backendsdispatchers
a327de1
to
313008f
Compare
313008f
to
3a8d02c
Compare
datastore/http/http.go
Outdated
} | ||
|
||
func (p *Producer) getRecordChannel(vin string) chan *telemetry.Record { | ||
return p.workerChannels[vinToHash(vin)%uint32(len(p.workerChannels))] |
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.
hmm. I agree with #91 (comment) but at the same time, it could be possible that not all channels will be used (all vins getting partitioned into a single channel or only one vin to stream and increasing worker channel will not make a difference). I guess this is fine since its not going to be recommended producer but maybe we should add a comment in readme about relation between worker channels and vins associated with this account
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 agree it's good to call this out. I have added comentary about this to WorkerCount
in Config
. We could also add a label to http_produce_buffer_size
metric calling out size of each worker's buffer. Want me to add this?
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 sure!
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.
Added 👍
3a8d02c
to
622a612
Compare
contentType := protobufContentType | ||
if p.produceDecodedRecords { | ||
contentType = jsonContentType | ||
} | ||
req.Header.Set("Content-Type", contentType) |
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.
Updated to set Content-Type
based on json vs protobuf. I'm using application/x-protobuf
when it's the proto, but there are not clear standards on this available. Open to suggestion on this.
622a612
to
4630a2a
Compare
4630a2a
to
84e8562
Compare
Description
Creating an HTTP dispatcher intended to be used for small-scale personal projects. Data received from a vehicle is delivered through a POST request to the specified http server. The body of this request is JSON to make for easy usage.
The existing dispatchers are great for high reliability, high throughput systems, but for an owner wanting to stream data from their own vehicles, setting up and maintaining this infrastructure is a bit much. This allows enthusiasts to create a simple web server which receives the data.
As called out in the updated Readme, this should not be used in production grade systems. Failed messages are discarded and sending http requests does not scale well.
Type of change
Please select all options that apply to this change:
Checklist:
Confirm you have completed the following steps:
Additional Notes
I am naming the integration tests container so it can be referenced through Docker DNS.
JSON payload format: