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

create http dispatcher #91

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

create http dispatcher #91

wants to merge 1 commit into from

Conversation

patrickdemers6
Copy link
Collaborator

@patrickdemers6 patrickdemers6 commented Dec 20, 2023

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:

  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Bug fix (non-breaking change which fixes an issue)
  • Documentation update

Checklist:

Confirm you have completed the following steps:

  • My code follows the style of this project.
  • I have performed a self-review of my code.
  • I have made corresponding updates to the documentation.
  • I have added/updated unit tests to cover my changes.
  • I have added/updated integration tests to cover my changes.

Additional Notes

I am naming the integration tests container so it can be referenced through Docker DNS.

JSON payload format:

{
  "createdAt": "2023-12-21T02:46:51.386927610Z",
  "vin": "device-1"
  "data": [
    {
      "key": "VehicleName",
      "value": {
        "stringValue": "My Test Vehicle"
      }
    },
    {
      "key": "Location",
      "value": {
        "locationValue": {
          "latitude": -37.412374,
          "longitude": 122.145867
        }
      }
    }
  ]
}

namespace: namespace,
metricsCollector: metricsCollector,
records: requests,
address: config.Address,
Copy link
Collaborator

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

Copy link
Collaborator Author

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?

Copy link
Collaborator

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?

Copy link
Collaborator Author

@patrickdemers6 patrickdemers6 Dec 21, 2023

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.

datastore/http/http.go Outdated Show resolved Hide resolved
datastore/http/http.go Outdated Show resolved Hide resolved
datastore/http/http.go Outdated Show resolved Hide resolved
datastore/http/http.go Outdated Show resolved Hide resolved
test/integration/test.sh Show resolved Hide resolved
datastore/http/http.go Outdated Show resolved Hide resolved
datastore/simple/logger.go Outdated Show resolved Hide resolved
@@ -14,51 +15,28 @@ import (
var _ = Describe("Proto Logger", func() {
var (
protoLogger *simple.ProtoLogger
logger *logrus.Logger
Copy link
Collaborator

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?

Copy link
Collaborator Author

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.

datastore/simple/logger_test.go Outdated Show resolved Hide resolved
@patrickdemers6
Copy link
Collaborator Author

@agbpatro / @aaronpkahn could I get eyes on this again?

@agbpatro
Copy link
Collaborator

agbpatro commented Jan 8, 2024

@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

@hauserkristof
Copy link

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

Copy link
Collaborator

@agbpatro agbpatro left a 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?


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()
Copy link
Collaborator

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

datastore/http/http.go Outdated Show resolved Hide resolved
datastore/http/http.go Outdated Show resolved Hide resolved
datastore/http/http.go Outdated Show resolved Hide resolved
}

func (p *Producer) getRecordChannel(vin string) chan *telemetry.Record {
return p.workerChannels[vinToHash(vin)%uint32(len(p.workerChannels))]
Copy link
Collaborator

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

Copy link
Collaborator Author

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes sure!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added 👍

Comment on lines +161 to +171
contentType := protobufContentType
if p.produceDecodedRecords {
contentType = jsonContentType
}
req.Header.Set("Content-Type", contentType)
Copy link
Collaborator Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants